Prefer JavaScript UMD modules spec #1028
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Waiting on #1027 to get the tests passing |
It appears that Webpack is already handling the UMD import/export specification correctly when compiling the assets within Amber. In the amber.js websocket file, both UMD and common JS are present, and non-identical as both `class Channel` and `class Socket` are exported in the UMD format but only Socket is exported as CommonJS. A side-effect of this is that the `module.exports` object is transpiled into the bundle as Webpack has already defaulted to using UMD. This commit simply removes the CommonJS export definition.
jackturnbull
force-pushed
the
bug/single-js-export-format
branch
from
2df99ca to
ef0b73e
Compare
|
Sorry, appears that the rebase has squashed the revew! Tests passing at least. |
robacarp
approved these changes
Jan 8, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
It appears that Webpack is already handling the UMD import/export specification correctly when compiling the assets within Amber. In the amber.js websocket file, both UMD and common JS are present, and non-identical as both
class Channelandclass Socketare exported in the UMD format but only Socket is exported as CommonJS. A side-effect of this is that themodule.exportsobject is transpiled into the bundle as Webpack has already defaulted to using UMD.This commit simply removes the CommonJS export definition.
Alternate Designs
None / N/A
The CommonJS pattern could be chosen, but with Webpack support for UMD I don't believe this would be worthwhile.
Benefits
This was spotted as I had changed my JavaScript bundler to Rollup.js but the output was evidently not handled as graciously as Webpack's as I had a
module.exportsremaining in the output withoutmoduleitself being defined.The benefit being that this opens the floor to using other JS bundlers that also contain the same issue.
Possible Drawbacks
None as far as I can tell. I've stepped through the output of the webpack bundles before and after this change and in both cases both the Socket class and the Channel class are the only two modules passed into the bootstrap function, although if anyone has more context for why the module.exports definitions remain that would help also!
Summary of the output bundles: