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

Build minified and unminified bundles #253

Merged
merged 2 commits into from
Jan 10, 2017
Merged

Build minified and unminified bundles #253

merged 2 commits into from
Jan 10, 2017

Conversation

pimterry
Copy link
Contributor

@pimterry pimterry commented Jan 10, 2017

I've defaulted to the minified version (by setting that as .browser in package.json), which I think is probably preferable, but up for debate.

@pimterry pimterry mentioned this pull request Jan 10, 2017
@emirotin
Copy link
Contributor

Debatable indeed because the browser field is always used by the bundler like browserify/webpack.
I've also suddenly realized that we'd want to use the source version in our own code (to dedupe the bluebird/lodash/etc) which will contradict with the browser field (as it will provide the UMD bundle instead).
What do you think?

@pimterry
Copy link
Contributor Author

Mmm, this is interesting. I haven't really been thinking about downstream bundling concerns, because if you're bundling downstream generally you don't want us to provide a browser bundle.

As you mention for in the UI case, for example, we really want to just depend on the raw source so that everything gets deduped and you can control bundling yourself, and I think that probably holds for nearly 100% of other bundling users too. I can't think of any reason you'd normally want the bundle in these cases.

Given that, the use case for the bundle is in non-commonjs bundled environments - raw globals, requirejs, maybe more exotic things. I don't think any of other options use the browser field, so we should actually just drop it entirely, and have everything that uses package.json get the non-bundled version, and people use the bundled version only when they're setting things up manually. Does that sound reasonable?

@emirotin
Copy link
Contributor

emirotin commented Jan 10, 2017 via email

@@ -2,6 +2,8 @@ karmaConfig = require('resin-config-karma')
packageJSON = require('./package.json')
{ loadEnv } = require('./tests/util')

BROWSER_BUNDLE = "build/resin-browser.js"

Choose a reason for hiding this comment

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

Unnecessary double quotes are forbidden.

@pimterry
Copy link
Contributor Author

Ah, wait. There's still the other module-exclusion bundling problem. The config in this module currently lets us exclude some of its deps, which keeps downstream bundles smaller, and we don't share any of that info if we don't use the bundle when bundling downstream. Interesting...

This is where the browser field is supposed to come in: if we point it at source that never even require()s the modules we want to exclude (i.e. statically never even considers requiring them, rather than just hiding it behind an IS_BROWSER test) then browserify won't ever try to include them at all, even downstream. In principle that's doable with some refactoring, but it's a fairly wide ranging change. We'd need to build browser/non-browser versions of the relevant bits of this module and the other used resin modules, so that as browserify walks the tree it never sees rindle, for example.

For now though, as above, I think we still want to drop the browser field. Note that this means we're expecting downstream consumers to manually exclude modules though (but that's ok since the main downstream consumer right away will be us). We can pick up a separate task later to provide browserify/webpack-friendly builds with automagically exclude the right dependencies, since that's a larger job and it should be easily backward compatible anyway.

I've now pushed a browser-less update. Let me know if you're happy with all this and I'll merge and file an issue to look at the rest later.

@emirotin
Copy link
Contributor

emirotin commented Jan 10, 2017 via email

@pimterry
Copy link
Contributor Author

Readme note added, merging.

@pimterry pimterry merged commit 03d04ca into sdk-browser Jan 10, 2017
@pimterry pimterry deleted the non-min-bundle branch January 10, 2017 17:39
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.

3 participants