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

(#5545) - remove lerna #5550

Closed
wants to merge 3 commits into from
Closed

(#5545) - remove lerna #5550

wants to merge 3 commits into from

Conversation

nolanlawson
Copy link
Member

It turns out there were only a few changes needed to remove lerna bootstrap entirely:

  • move packages to packages/node_modules
  • fix .gitignore so that it only ignores the top-level node_modules
  • add all dependencies from sub-packages to the top-level package

This change only removes Lerna. There are still some remaining TODOs that can be tackled in a future PR, such as not duplicating dependencies between sub-packages and the main package, but for now this is the simplest PR.

@nolanlawson
Copy link
Member Author

Oh I should also mention that this fully re-enables GreenKeeper, although we will still need to manually update versions in the sub-packages for changed dependencies (not dev dependencies) until I can fix the TODO described above.

"pouchdb-promise": "5.5.0-prerelease",
"pouchdb-replication": "5.5.0-prerelease",
"pouchdb-utils": "5.5.0-prerelease"
},
Copy link
Member Author

Choose a reason for hiding this comment

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

devDependencies are no longer needed inside of the sub-packages, since everything is handled either at the top-level node_modules or in packages/node_modules. These devDependencies previously only existed so that lerna bootstrap would work correctly.

@nolanlawson
Copy link
Member Author

Looks like this shaves about 30 seconds off of each build. :)

@nolanlawson
Copy link
Member Author

Well unfortunately this broke the 0.10 build because of my too-clever-by-half "build in Node 6, test in Node 0.10" thing. Looks like Rollup expects Map to be there and barfs if it's not.

@nolanlawson
Copy link
Member Author

Hm, this is a very hard problem to solve. I can't think of a way to extract just the test suite (which doesn't require Rollup), and then install it and run it in Node 0.10. Maybe it would be easier just to open a PR on Rollup to support 0.10.

@nolanlawson
Copy link
Member Author

Uggg, I could solve this by just dumping all the devDeps into dependencies, because then npm install --production wouldn't run npm run build, but it's kinda ugly to slam them all together like that. We could also disable the prepublish so that it doesn't run after npm install. Not sure what to do here.

@daleharvey
Copy link
Member

'0.10' becomes entirely unsupported in 50 days, I think its fine for us to drop support

@nolanlawson
Copy link
Member Author

Big 👍 to that.

@nolanlawson
Copy link
Member Author

OK, removed 0.10 entirely and moved back to the old system of using the same Node for building and testing.

@nolanlawson
Copy link
Member Author

lol wow eslint doesn't work in either 0.11 or 0.12 because it uses let. this is ridiculous

@nolanlawson
Copy link
Member Author

I propose we move linting to a separate build entirely. There's no reason to run it for each and every build anyway.

@nolanlawson
Copy link
Member Author

Oh for crying out loud, forgot that eslint runs after each test. Let's just add 0.11 and 0.12 to allowed failures for now.

@nolanlawson
Copy link
Member Author

Now that Firefox is fixed, trying again...

@nolanlawson
Copy link
Member Author

0.11 and 0.12 are failing in all tests because we run eslint. I decided to drop all of 0.10, 0.11, and 0.12 entirely. There's no easy way I can see to support 0.10-0.12 if eslint has decided their minimum baseline is 4.

Maybe in the future we can figure out how to write a patch to do the "build in one version, test in another" thing again, but right now it's just getting too difficult to support anything less than 4.

OTOH I added a test for Node 4 itself, so we're currently testing in 4, 5, and stable, with an allowed failure on stable.

@daleharvey
Copy link
Member

So I am 👍 on that, these node versions are going entirely unsupported (no security upgrades) very shortly so if its blocking improvements then happy to drop support.

Would like to ping @yaronyg though, if you absolutely need 0.X then would like to give you the chance to do the extra work to get it working here, or at least notice to fork, hopefully you can use >=4 by now though

@nolanlawson
Copy link
Member Author

All tests are passing except code coverage. I will look into it.

No coverage information was collected, exit without writing coverage information
ERROR: No coverage files found.

@nolanlawson
Copy link
Member Author

OK, had to fix Istanbul to not-ignore packages/node_modules. Now hopefully this should all be good and I can move on to the other stuff that's blocking 6.0.0. 😛

fast_finish: true
include:
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit, could we move this to the top of the matrix: section, I was confused that allowed_failures had this section inside of it

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

@daleharvey
Copy link
Member

Had one tiny comment on the travis.yml but otherwise this is great, faster + lesss complicated builds is awesome.

One other tiny question, so if we want to use a package in a submodule, we need to add it to both package.json and that submodule? if so it seems a note in the docs somewhere would be useful.

Would still like to hear from Yaron just in case, but 👍 maybe wait till monday before merging?

@daleharvey
Copy link
Member

lol ok just read #5583 so can ignore the question :)

@nolanlawson
Copy link
Member Author

Made the .travis.yml change on the way in, see 9b51141

@yaronyg
Copy link
Member

yaronyg commented Aug 22, 2016

@daleharvey Unfortunately we can't move to Node 4. The reason is that we are running on JXcore which is the only version of node that currently runs on mobile phones and it is based on a hybrid of Node 0.10 and 0.12. We are updating it to do things like support the latest security patches for OpenSSL. But until we figure out some way to get mainline Node working on Android and iOS we have no option but to stick with JXcore. We are looking to put in resources that could monitor PouchDB failures on Node 0.10/0.12 and submit fixes.

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.

None yet

3 participants