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

Add UMD wrapper for distribution #21

Closed
nrabinowitz opened this issue Oct 3, 2018 · 9 comments
Closed

Add UMD wrapper for distribution #21

nrabinowitz opened this issue Oct 3, 2018 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@nrabinowitz
Copy link
Collaborator

This might make the library a bit more accessible, and easier to use in Observable etc.

@nrabinowitz nrabinowitz self-assigned this Oct 3, 2018
@nrabinowitz nrabinowitz added the enhancement New feature or request label Oct 3, 2018
@GordonSmith
Copy link

GordonSmith commented Jan 10, 2019

I have some observations when trying to consume this library in a browser environment:

  • Missing dependencies - building from scratch in a clean clone fails inside .build-emscripten.sh (I wanted to play with some of the emcc flags)
  • Node dependencies - there are internal dependencies on "fs" and "path" inside the emscripten generated code, which causes issues with downstream bundlers
  • libh3.js has an issue with "use strict" which prevents it from being minified with UglifyJS (and subsequently webpack

I don't mind contributing, but the approach I would take may not fit in with your preferred toolsets:

  • Switch to es6 modules (use "import")
  • Remove the buble dependency and use either TypeScript (my preference) or Babel to transpile to es5.
  • Use RollupJS (my preference) or WebPack to create a UMD compatible bundle.
  • Tweak the emcc options to resolve the above node dependency and strict issues (if this is possible - I haven't used emscripten before)

If anyone else is interested the following WebPack configuration + es6 index appears to work:

lib-es6/index.js

import * as h3js from "h3-js";
export var geoToH3 = h3js.geoToH3;

webpack.config.js

const path = require('path');

module.exports = {
    entry: {
        "index": "./lib-es6/index.js"
    },
    output: {
        path: path.resolve(__dirname, 'dist/'),
        publicPath: "dist/",
        filename: "[name].js",
        libraryTarget: "umd",
        library: "h3-js-umd"
    },
    node: {
        fs: 'empty',
        path: 'empty'
    },
    mode: "production",
    optimization: {
        minimize: false
    }
};

@nrabinowitz
Copy link
Collaborator Author

Thanks for the notes. I think the Webpack issues may be idiosyncratic depending on your Webpack setup, as we have a number of applications using Webpack and bundling h3-js without issues.

Missing dependencies - building from scratch in a clean clone fails inside .build-emscripten.sh (I wanted to play with some of the emcc flags)

This shouldn't be the case - we build from scratch in Travis. You do need docker installed locally, and you need to run yarn init-docker before you run yarn build-emscripten (if your docker container isn't already running). This is covered in the README.

Node dependencies - there are internal dependencies on "fs" and "path" inside the emscripten generated code, which causes issues with downstream bundlers

I haven't seen issues with this, but it may be something we can turn off in Emscripten. Can you file a separate issue for this?

libh3.js has an issue with "use strict" which prevents it from being minified with UglifyJS (and subsequently webpack

Can you describe this more, and possibly file a separate issue?

@dfellis
Copy link
Collaborator

dfellis commented Jan 10, 2019

The last issue isn't really an issue -- the libh3.js file is the output of the emscripten compiler into the asm.js format. It should NOT be minified because that will break the relatively fragile asm.js detection logic in the browsers and cause a performance drop.

When I have some time to get an h3-wasm written, that concern will go away as the payload will be binary and more compact than a minifier could push it, anyways.

@nrabinowitz
Copy link
Collaborator Author

When I have some time to get an h3-wasm written, that concern will go away as the payload will be binary and more compact than a minifier could push it, anyways.

Not sure that's true, as I think most consumers will still use the JS version as the default, since the WebAssembly version necessarily incurs more complexity (e.g. async loading of the binary) until better browser support shows up - plus Node or isomorphic usage will still require JS.

@dfellis
Copy link
Collaborator

dfellis commented Jan 10, 2019

Node users should be using h3-node instead for the performance benefits, and modern browsers should (when ready) use h3-wasm for similar reasons.

I think this package compiling to asm.js, which is also compatible with any regular ES5 JS engine, would become the fallback implementation for some meta-package that detects which environment the user is in (by feature detection of the WebAssembly and N-API APIs) and chooses the correct version. The majority of real-world use-cases right now (Chrome/Firefox/Safari browsers, Node backends) would not use h3-js in that hypothetical scenario.

@nrabinowitz
Copy link
Collaborator Author

Node users should be using h3-node instead for the performance benefits, and modern browsers should (when ready) use h3-wasm for similar reasons.

I think there are going to be cases where either requirements for isomorphic code and/or the comparative simplicity of the native JS version are going to significantly outweigh the perf improvements in using h3-node or h3-wasm. My guess is that in practice h3-js will continue to be the recommended lib due to simplicity of installation and use, and the other two will be primarily for consumers interested in trading simplicity for performance.

@nrabinowitz
Copy link
Collaborator Author

To address @GordonSmith's suggestions:

Switch to es6 modules (use "import")

Agreed. I've started on this, but the Emscripten piece adds complexity to the build.

Remove the buble dependency and use either TypeScript (my preference) or Babel to transpile to es5.

Not sure is this is needed - buble + rollup should be fine, and is lighter-weight and simpler than Babel. We're unlikely to move to TS any time soon, though I'd like to provide TS typedefs for the lib.

Use RollupJS (my preference) or WebPack to create a UMD compatible bundle.

I've started on this, but got blocked by Emscripten already wrapping the core lib in UMD. Planning to pursue in the next month or so.

Tweak the emcc options to resolve the above node dependency and strict issues (if this is possible - I haven't used emscripten before)

We can look into this. Agree with @dfellis that further minification is probably a bad idea, so the use strict part may not be an issue.

@GordonSmith
Copy link

Thanks for the informative posts:

  • I was able to complete the clean build - I had some errant \r line endings in .build-emscripten.sh and H3_VERSION causing the issue (am running in WSL on windows).
  • Had I been developing in Node, I would probably prefer to use h3-node (had I known about it) - it may be worth mentioning in the readme?
  • Based on the current sources, my WebPack config is actually as good as it gets for now.

I don't believe I need to open any new issues at this time?

@dfellis
Copy link
Collaborator

dfellis commented Jan 11, 2019

So h3-node is noted in the bindings documentation for H3. There's no description on why you should use one or the other, but as the discussion above shows, that's likely to have differing opinions between people, anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants