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

Upgrade lerna setup #236

Closed
wants to merge 4 commits into from
Closed

Conversation

vlad-zhukov
Copy link
Collaborator

@vlad-zhukov vlad-zhukov commented Oct 30, 2017

I made a few changes we've discussed in #226, #231, #204
Closes #204

What have I done:

  • upgraded lerna to 2.4.0 (Improve build #226 (comment))
  • added --hoist flag to lerna bootstrap (Try lerna hoisting #204) and ran it with the latest npm@5.5.1 (it's responsible for upgrading all deps and it's not possible to revert it, we have to use yarn if we want more predictable behaviour in this department)
  • moved the sample-app into examples folder (Fix validation error with default uglify options #231 (review))
  • added --parallel flag to lerna run test, it should be faster now (we haven't discussed it before but I find it interesting to try)
  • tried to fix a skipped test in the sample-app

This is a WIP because I can't make tests pass locally due to a weird error in the elm block. Would be great if someone could help me with it because I've no experience with elm.

P.S.: Sorry for a large PR, I can try to split it if you want.

@@ -46,16 +46,16 @@ test('it exports the production config', t => {
process.env.NODE_ENV = NODE_ENV
})

test.skip('it builds', t => {
test.only('it builds', async t => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to remove it, will fix later

@@ -1,11 +1,12 @@
{
"private": true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I keep it?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the monorepo root package.json should be private. Only the packages it contains should be published, never the monorepo root 😉

testHtml()
testCss()
testJs()
t.end()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests are failing because t.end() is not needed.

@andywer
Copy link
Owner

andywer commented Nov 1, 2017

Thx for your PR, Vlad. Good ideas!

I'd just like to move updating the dependencies and refactoring the ugly callback-based test into separate PRs.

Жуков Владислав Юрьевич and others added 3 commits January 27, 2018 01:38
@vlad-zhukov
Copy link
Collaborator Author

vlad-zhukov commented Jan 26, 2018

Merged with master, updated lerna to latest, bumped a few deps and fixed tests.

We have to update certain deps in this PR because of the way lerna hoists them. For instance, if one package has "webpack@^3" and another has "webpack@^2", lerna will hoist "webpack@^3" to the root folder and won't install "webpack@^2" at all. This is very weird and I dislike such behaviour, yarn workspaces never do this.

@vlad-zhukov vlad-zhukov changed the title [WIP] Upgrade lerna setup Upgrade lerna setup Jan 26, 2018
@vlad-zhukov
Copy link
Collaborator Author

Can we merge it? It's blocking me from doing more awesome stuff! 🙃

@andywer
Copy link
Owner

andywer commented Feb 7, 2018

I know that the package-lock.json is nasty, but it is supposed to be committed.

@andywer
Copy link
Owner

andywer commented Feb 7, 2018

Have been experimenting with yarn workspaces and oao a bit recently (see andywer/shutter, for instance). Feels simpler and cleaner, actually.

Used oao instead of lerna, because lerna's "run script in all packages" output often is hardly readable.
Maybe we can discuss such an option together on gitter or in a separate issue.

@vlad-zhukov
Copy link
Collaborator Author

Sure, that would be my next steps after this PR. We should also consider bolt instead of lerna by the way.

@andywer
Copy link
Owner

andywer commented Feb 9, 2018

Can barely tell the difference between bolt and oao ^^ But sure.

But this PR is really hard to review...

@vlad-zhukov vlad-zhukov mentioned this pull request Apr 21, 2018
17 tasks
@dmitmel
Copy link
Contributor

dmitmel commented Apr 24, 2018

@andywer @vlad-zhukov We now use Yarn workspaces, so this PR can be closed.

@vlad-zhukov vlad-zhukov deleted the lerna-upgrade branch April 24, 2018 19:47
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.

Try lerna hoisting
3 participants