Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monorepo #729

Merged
merged 6 commits into from
Jul 5, 2018
Merged

Monorepo #729

merged 6 commits into from
Jul 5, 2018

Conversation

HenriBeck
Copy link
Member

@HenriBeck HenriBeck commented Jun 16, 2018

What is already done:

  • Moved whole jss package into packages/jss
  • Initialized lerna
  • Added universal build scripts
  • Karma runs the test succesfully

Still left to do:

  • There is a problem with the snapshoting

- Initialized lerna
- Added universal build scripts

Signed-off-by: Henri Beck <[email protected]>
@HenriBeck HenriBeck mentioned this pull request Jun 16, 2018
Henri Beck added 2 commits June 16, 2018 20:22
Signed-off-by: Henri Beck <[email protected]>
@HenriBeck
Copy link
Member Author

Fixed snapshot testing

@HenriBeck
Copy link
Member Author

I don't know why we were building the test files before, but I haven't added that yet.

@HenriBeck HenriBeck requested review from kof and TrySound June 18, 2018 13:01
@kof
Copy link
Member

kof commented Jun 18, 2018

@HenriBeck we have this tests/index.html, which is using the tmp/tests.js. I think we can get rid of it. The use case was to run tests from a static page, but we can also run it through the local tests server once it is running. It was easy to just open a html file without any server on any browser.

@kof
Copy link
Member

kof commented Jun 19, 2018

@HenriBeck let me know when to try this out

@HenriBeck
Copy link
Member Author

@kof should be ready to try out

@HenriBeck
Copy link
Member Author

HenriBeck commented Jun 20, 2018

The problem is when we merge any pr, we create conflicts because literally, every file has changed. So if we could move forward before any other PRs are merged it would be great.

@kof
Copy link
Member

kof commented Jun 20, 2018

oh yeah, sorry for that, we need to merge it asap, I will look into it today

@kof
Copy link
Member

kof commented Jun 20, 2018

are you available on some chat? in case I have things to discuss

@HenriBeck
Copy link
Member Author

HenriBeck commented Jun 20, 2018

you can just email me at [email protected] or just post it here

@@ -7,3 +7,4 @@ lib
dist
.env
coverage
packages/**/lincense.md
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a license file in each package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just copies it when the build script runs from the root path. I think every npm package should have a LICENSE file, I don't think we lose anything without it.

@@ -4,4 +4,4 @@ JSS core library is a rendering engine. We aim to make it small, fast and unopin
It is often too low level for applications.
That's why there is a bunch of higher level libraries on top of it, which provide more opinionated sugar APIs and framework integrations.

[See all projects](./projects.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

driveby commits like this making PR larger and harder to merge

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

driveby commits?

@@ -0,0 +1,40 @@
/* eslint-disable no-console */
Copy link
Member

@kof kof Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this file instead of just using those commands directly in npm scripts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it to run those scripts over each package? doesn't lerna has something to do that in a more concise way? any other options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we would need to install babel-cli for every package, there is no way to use those commands inside npm scripts without installing it inside every package.

That way we only need one script and not have multiple build scripts for every package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about ../../node_modules/.bin/{script}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, we would need a build:lib, build:dist and maybe build:flow script in every package.
I think this one is more maintainable. This approach is also used by storybook.

@kof
Copy link
Member

kof commented Jul 5, 2018

Ok, found again time, working on it.

@kof
Copy link
Member

kof commented Jul 5, 2018

there are so many questions open that I think we can't make it in one PR and we need to start merging it and iterate

@kof
Copy link
Member

kof commented Jul 5, 2018

Do we want to have a single version number for all plugins or separate?

@kof kof merged commit 9b7daa7 into master Jul 5, 2018
@kof
Copy link
Member

kof commented Jul 5, 2018

merged 🎉

@HenriBeck
Copy link
Member Author

I think we should have one single version. One single version for the core and the plugins which should make it clear which version you need to install when installing a plugin.

@HenriBeck
Copy link
Member Author

I would start importing the plugins then next. Should find some time on the weekend for that.

@kof
Copy link
Member

kof commented Jul 5, 2018

yeah, me too, even though react-jss could be a separate version, I think its easier that way

@kof
Copy link
Member

kof commented Jul 5, 2018

On the related note, created

#745

#744

@TrySound TrySound deleted the monorepo branch July 5, 2018 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants