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.

Sign up for GitHub

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

Jump to bottom

Improve packaging to better support module bundlers #414

Merged
merged 17 commits into from
Sep 27, 2018
Merged

Improve packaging to better support module bundlers #414

merged 17 commits into from
Sep 27, 2018

Conversation

lutovich
Copy link
Contributor

@lutovich lutovich commented Sep 20, 2018 •
edited
Loading

  • Replace local copy of text-encoding library with a dependency
  • Split Node and browser components into separate modules and export them from src/v1/internal/node/index.js and src/v1/internal/browser/index.js
  • Make all files import node/index.js by default
  • Make UMD module build skip Node components
  • Split Node and browser specific tests into separate folders test/internal/node and test/internal/browser
  • Make UMD module tests skip tests in test/node
  • Make Node tests skip test/browser
  • Use "browser" field in package.json to make bundlers replace Node components with browser components
  • Use browserify transform to replace require('./node') with require('./browser') when building UMD modules
  • Remove runtime feature detection in favor of "main" and "browser" fields in package.json and require rewriting for UMD module
  • Add integration test with Webpack
  • Exclude test UMD bundle neo4j-web.test.js from the npm package
  • Migrate couple tests to ES6 syntax
  • Move DummyChannel to test scope

Resolves #396

@lutovich
Replace local copy of text-encoding with dependency
c2a42b4 
Removed copied source of the text-encoding library and added a regular
npm dependency instead.
@lutovich
Replace couple require calls with imports
a549aa4
@lutovich
Move couple tests to ES6 and dummy channel to test scope
1358ae5
@lutovich
Make driver package work with Webpack
2e4e176 
Improved feature detection and tweaked package.json in order to make
Webpack not load NodeJS modules. Feature detection is also changed to
account for possibly empty Node modules, like net ot dns. Previous
packaging was the same for Node and browser and feature detection
happened at runtime. Webpack, however, tried to resolve modules at
compile time and failed. New "browser" entry in package.json tells
bundlers to return empty objects for Node modules.
@lutovich
Split Node and browser components into separate modules
b910f28 
This makes it easier to override Node modules in browser environment.
@lutovich
Improve packaging and split browser and Node-specific code
ef27057 
This commit separates components that depend on browser environment
from components that use Node features. No separation was previously
needed because code used runtime feature detection. Such feature
detection made it impossible for bundlers, like Webpack, to strip
away Node-specific code. Presence of it caused failures at build time
and made it hard to use the driver package.

Driver contains couple env-dependent components:
 * network channel (`WebSocket` in browser, `net.Socket` in Node)
 * byte buffer (`ArrayBuffer` in browser, `buffer.Buffer` in Node)
 * host name resolver (no-op in browser, `dns` lookup in Node)
 * UTF8 encoding & decoding (`text-encoding` library in browser, `StringDecoder` in Node)

Implementations of these components now live in separate folders
`src/internal/node` and `src/internal/browser`. Both contain `index.js`
files that export the same set of components and follow the same
"interface". Env-dependent tests are also split into separate folders.
All sources import/require `node/index.js` by default.

`package.json` now contains a "browser" field which instructs bundlers
to replace all imports/require of `node/index.js` with `browser/index.js`
for client-side apps. This way code that depends on Node APIs will not
even be included in the resulting bundle.

Gulp build for UMD `neo4j-web.js` and `neo4j-web.test.js` bundles
excludes `node` folders and only includes `browser`. It also replaces
all require calls of "/node" to "/browser". Dedicated browserify
transformation is added for this to the bundling process. Resulting
bundles no longer contain code that depends on Node APIs.
@lutovich
Small cleanup of gulp build script
32400f2 
Removed ignores of "external" folder because it no longer exists. Keep
babel config in one place.
@lutovich
Update package-lock
533ee31
@lutovich
Add unit tests for 'encrypted' and 'trust' config validation
3fad54f
@lutovich
Remove feature detection of TRUST_ALL_CERTIFICATES
f99f704 
It requires `TlsSocket.getPeerCertificate()` function to work, which
is available since Node v0.11.4.
@lutovich
Exclude browser test bundle from package
f8eb767 
Test bundle for browser environment `neo4j-web.test.js` is generated
at build time and contains driver code and test code to execute. It
was added to `lib/browser` directory and ended up included in the npm
package.

This commit makes build process generate the test bundle in
`build/browser` which is excluded from package. Also removed gulp file
from package.
@lutovich
Add comments about env-dependent APIs
7bcac41
@lutovich
Add integration test with Webpack
d232b6f 
To verify that driver can be used in a browser app. This makes sure no
NodeJS APIs are used unconditionally. Webpack build fails for such
APIs at compile time. Webpack uses "browser" field from package.json to
replace Node components with browser components. All environment-dependent
code should only be exposed from `node/index.js` and `browser/index.js`.
@lutovich
Test package only in Node environment
7c2f156 
It depends on Webpack which can't be included in a browser
test UMD bundle.
@lutovich
Only execute direct driver tests that use stub server in Node
aeedec1
ali-ince
ali-ince approved these changes Sep 24, 2018
View reviewed changes
Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

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

LGTM, I've also verified it with a very dumb test drive with create-react-app.

oskarhane
oskarhane approved these changes Sep 26, 2018
View reviewed changes
Copy link
Member

@oskarhane oskarhane left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Much appreciated change!
This looks good to me, thank you.

I've tried it in neo4j-browser which is using webpack 3 and the browser field overrides in package.json works fine. After this change we can remove our (neo4j-browser) webpack alias workaround:

'neo4j-driver': 'neo4j-driver/lib/browser/neo4j-web.min.js',

which in turn should make the bundled code for neo4j-browser better structured.

So 👍

@lutovich
Merge branch 1.7 into 1.7-webpack-support
9424ed5
@lutovich
Fix checking of window global variable in strict mode
1fe6026 
Previous version of the check failed in strict mode with error
"window is not defined". Strict mode is default in Webpack.
@lutovich lutovich merged commit 15deb86 into neo4j:1.7 Sep 27, 2018
@lutovich lutovich deleted the 1.7-webpack-support branch September 27, 2018 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oskarhane oskarhane oskarhane approved these changes

@ali-ince ali-ince ali-ince approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lutovich @oskarhane @ali-ince