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

(#5128) - split PouchDB into a monorepo with separate packages #5178

Merged
merged 1 commit into from May 20, 2016

Conversation

nolanlawson
Copy link
Member

@nolanlawson nolanlawson commented May 19, 2016

This ought to pass in Travis. The monorepo is ready.

This is a huge code change with lots of moving parts, so I'm going to try to summarize the major changes and especially the non-obvious stuff:

New project architecture

All packages have src and write to lib. They all use src/index.js as the starting point, never src/index-browser.js, mostly to make the Rollup scripts simpler (only one entry point). The "browser" field in their package.jsons have to list all src/ and lib/ switches in order to satisfy both Rollup and Browserify.

All docs, testing, etc. is done at the top-level. Sometimes dependencies need to be duplicated here, e.g. we use request in our build scripts, but this is not the same request used by pouchdb itself. (Although I gave them the same version, to keep it simple.)

Build scripts

npm run build, as before, is a prepublish step that runs after npm install. It uses lerna bootstrap to link all packages together, and then builds all sub-modules in one Node script called bin/build-modules.js.

Except for pouchdb, every module is built with bin/build-module.js, which uses only Rollup, so it's quite fast. pouchdb is built with a more elaborate Rollup+Browserify+Uglify script in bin/build-pouchdb.js. No build scripts are in the sub-packages; they're all up top in bin. To build all modules without running Lerna, run npm run build-modules.

Also, npm run dev is very efficient in that it avoids rebuilding stuff it doesn't need to (e.g. uglify, dist/, derequire, bundle-collapser). There is also a BUILD_NODE=1 that runs before the Node-only tests (npm t) to ensure that the browser build is not run, so it's much faster than a full build. See bin/build-pouchdb.js for more details.

Internal package structure

pouchdb-for-coverage is a private (non-published) package, marked by "private": true. So is the root package, which I called pouchdb-monorepo for fun.

Non-private internal packages are mostly divided based on large CommonJS dependencies they use (e.g. vuvuzela -> pouchdb-json, spark-md5 -> pouchdb-md5). Due to the way Rollup handles external CJS packages, it makes sense to break these up and not just include them in pouchdb-utils, because otherwise they are not properly tree-shaken. (I know, I know, it's annoying, but this works. Rollup is apparently not so great at tree-shaking non-ES modules.)

pouchdb has a special build script that bundles itself without directly depending on any of the sub-packages (instead they are devDependencies). This is to get the smallest possible bundle and to avoid a long install time for the core pouchdb package. It also means that the third-party dependencies have to be duplicated in pouchdb's package.json, but the advantage is that we can continue to ship pouchdb exactly as-is (even with optionalDependencies and so forth). We can revisit this later, but I thought it was a nice feature to pre-bundle pouchdb for our users.

Changes to testing

Integration test utils are now built from utils.js to utils-bundle.js. This is what allowed us to finally get rid of the PouchDB.utils, PouchDB.ajax, PouchDB.Errors etc. Unfortunately I had to expose a db._ajax method because some tests swap it out at runtime, and these couldn't be easily isolated to pouchdb-for-coverage. But hopefully the underscore communicates well enough that it's private.

pouchdb-for-coverage is exactly what it sounds like. It exposes all the internal APIs we need to test during unit/component tests so that we can reach full coverage. It is minimally different from the pouchdb package. It mostly just adds a bunch of extra packages to PouchDB.utils, PouchDB.Errors etc.

Babel is no longer needed, thanks to pouchdb-for-coverage. There's no need to build-as-modular-es5 anymore just to test coverage.

I had to do "allowed failure" for the Firefox perf test and ignore test.ajax.js. I can't figure out what's wrong with them, but they're not crucial in my eyes, and I can look into fixing them after the monorepo merge.

Publishing

I added an npm run set-version -- $version script, because I was annoyed with using tin and also needed a simple way to update all versions in all packages at once. This also allows us to keep our -prepublish convention; it's pretty easy to change whenever you want.

Lerna is essentially only used for linking. I'm not using it for publishing or anything else, because I wasn't convinced those features were really required. Also it's not used to build the modules, because I found I could write a much faster build script by using a single Node process instead of multiple npm scripts in separate processes.

Lerna is not used for publishing; I just iterate through all packages and do npm publish.

Documentation

The extras/ are deprecated. There's a new custom.md/custom.html file describing how to use the custom builds. Each README also contains minimal information about how to use the packages, mostly directing people to the custom.html page for full documentation. Each package has the requisite LICENSE/README/package.json.

TODOs

(All of these TODOs can be done post-merge IMO.)

GreenKeeper does not work anymore. I'm going to work with @janl to add monorepo support to GreenKeeper. In the meantime we can use npm update to manually update deps.

I'm using a fork of Lerna because it currently doesn't support ES6 modules with jsnext:main. This may change in the future, but in the meantime I'm comfortable maintaining this fork, since Lerna is really doing so little for us anyway (just linking everything together, i.e. lerna bootstrap).

Feedback

Please let me know what you think. Play around with the code, try it out, see if it seems reasonable.

Using the new codebase is as simple as npm install and npm run build, as before. npm run dev still works from the top level, as do the test scripts, docs scripts, etc.

@hzoo
Copy link

hzoo commented May 20, 2016

lerna should be able to do the set-version for yall as well

cc @thejameskyle.

@daleharvey
Copy link
Member

daleharvey commented May 20, 2016

Hrm, this is failing for me

~/src/pouchdb (monorepo-final)$ npm install

> pouchdb-monorepo@0.0.0 prepublish /Users/daleharvey/src/pouchdb
> npm run build


> pouchdb-monorepo@0.0.0 build /Users/daleharvey/src/pouchdb
> lerna bootstrap && npm run build-modules && npm run build-test

Lerna v2.0.0-beta.9
Linking all dependencies
Successfully bootstrapped 28 packages.

> pouchdb-monorepo@0.0.0 build-modules /Users/daleharvey/src/pouchdb
> node bin/build-modules.js

Building pouchdb...
Building pouchdb-adapter-fruitdown...
Building pouchdb-adapter-http...
Building pouchdb-adapter-indexeddb...
Building pouchdb-adapter-leveldb...
Building pouchdb-adapter-leveldb-core...
Building pouchdb-adapter-localstorage...
Building pouchdb-adapter-memory...
Building pouchdb-adapter-node-websql...
Building pouchdb-adapter-websql...
Building pouchdb-adapter-websql-core...
Building pouchdb-ajax...
Building pouchdb-binary-utils...
Building pouchdb-browser...
Building pouchdb-checkpointer...
Building pouchdb-core...
Building pouchdb-errors...
Building pouchdb-for-coverage...
Building pouchdb-generate-replication-id...
Building pouchdb-http...
Building pouchdb-json...
Building pouchdb-mapreduce...
Building pouchdb-mapreduce-utils...
Building pouchdb-md5...
Building pouchdb-node...
Building pouchdb-promise...
Building pouchdb-replication...
Building pouchdb-utils...
build error
TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.dirname (path.js:1324:5)
    at resolve (/Users/daleharvey/src/pouchdb/node_modules/browser-resolve/index.js:218:21)
    at /Users/daleharvey/src/pouchdb/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:46:5
    at tryToUnwrap (/Users/daleharvey/src/pouchdb/node_modules/lie/lib/index.js:171:5)
    at tryCatch (/Users/daleharvey/src/pouchdb/node_modules/lie/lib/index.js:183:17)
    at safelyResolveThenable (/Users/daleharvey/src/pouchdb/node_modules/lie/lib/index.js:174:16)
    at new Promise (/Users/daleharvey/src/pouchdb/node_modules/lie/lib/index.js:32:5)
    at resolveId (/Users/daleharvey/src/pouchdb/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:45:11)
    at /Users/daleharvey/src/pouchdb/node_modules/rollup/dist/rollup.js:2123:32
    at tryCatch (/Users/daleharvey/src/pouchdb/node_modules/rollup/dist/rollup.js:393:12)

npm ERR! Darwin 15.5.0
npm ERR! argv "/Users/daleharvey/.nvm/versions/node/v6.0.0/bin/node" "/Users/daleharvey/.nvm/versions/node/v6.0.0/bin/npm" "run" "build-modules"
npm ERR! node v6.0.0
npm ERR! npm  v3.8.6
npm ERR! code ELIFECYCLE
npm ERR! pouchdb-monorepo@0.0.0 build-modules: `node bin/build-modules.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the pouchdb-monorepo@0.0.0 build-modules script 'node bin/build-modules.js'.

@daleharvey
Copy link
Member

~/src/pouchdb (monorepo)$ node --version
v6.0.0
~/src/pouchdb (monorepo)$ npm --version
3.8.6

Mentioned on irc but preemtive 👍, I dont see any blockers and this is going to block any other patches to pouch so getting it merged quickly is probably for the best, awesome job on the monumental task

@nolanlawson
Copy link
Member Author

@daleharvey I can repro. node6 issue apparently.

@nolanlawson
Copy link
Member Author

This seems to be a bug in Rollup's rollup-plugin-node-resolve or possibly node-browser-resolve that only occurs in Node 6. I'm having a heck of a time trying to debug it with the node-inspector due to Promise/async inspection not working correctly.

How important is Node 6 support? I see endless complaints on Twitter of folks saying that huge portions of it are totally broken.

Offending line of code (opts.filename is undefined): https://github.com/defunctzombie/node-browser-resolve/blob/a238dd1ca8db9319cdd6b6340358a09f298c9156/index.js#L218

@nolanlawson
Copy link
Member Author

@daleharvey I have a PR out where we build in Node 5 but test in Node 0.10 through Node 6: #5186

I'm disappointed I didn't really solve the problem, but I'll work to follow up with Lerna and Rollup on the 0.10 and 6.0 failures. In the meantime, Node compat errors in our build tools shouldn't block us from shipping PouchDB; our users don't care what build tools we use to ship Pouch, just so long as it runs on their version of Node. In the meantime, we might have to say "PouchDB can only be built on Node LTS."

@daleharvey
Copy link
Member

Our users may not care but our developers and contributors do, Awesome job getting it tested across versions and I think what we have now is a reasonable tradeoff however I think the amount of work needed to get it working across versions is an indication that we are reaching the upper level of complexity that we should be at and with this merged we should definitely be looking at removing as much complexity as we can. A first step of that would be tracking the .10 and 6x bugs and removing the packing code when we can. (I would also like to see pouch's internal code de complexified)

Got this all installed on 5x and its working great, I would still like to see watchify improvements (it can be up to like 30 seconds to see changes in the code) but I will take a stab at that, I think this is good to merge now, awesome job :)

@nolanlawson
Copy link
Member Author

You're right; we should at least make sure it works on latest Node. I think 0.10 may be a lost cause at this point though.

I'll take a stab at watchify. I tried to get it down to as fast as it'll go, but there are definitely still improvements to make. At the very least, we should make dist/pouchdb.js fast to build.

I think the errors with 0.10 and 6 are less due to complexity and more due to bleeding-edge tools like Rollup and Lerna. I admit I have a bit of a weakness for the new-shiny. But that means it's my responsibility to go and contribute features to those tools and to ensure they don't inhibit new contributors.

Thanks for the review and feedback! I'll merge this and then follow up with build/watch improvements.

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