-
Notifications
You must be signed in to change notification settings - Fork 3.8k
chore: Simplify NPM package wrappers, improve chunk wrapper generator #5777
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,25 @@ const NAMESPACE_OBJECT = '$'; | |
| /** | ||
| * A list of chunks. Order matters: later chunks can depend on | ||
| * earlier ones, but not vice-versa. All chunks are assumed to depend | ||
| * on the first chunk. | ||
| * on the first chunk. Properties are as follows: | ||
| * | ||
| * - .name: the name of the chunk. Used to label it when describing | ||
| * it to Closure Compiler and forms the prefix of filename the chunk | ||
| * will be written to. | ||
| * - .entry: the source .js file which is the entrypoint for the | ||
| * chunk. | ||
| * - .exports: a variable or property that will (prefixed with | ||
| * NAMESPACE_OBJECT) be returned from the factory function and which | ||
| * (sans prefix) will be set in the global scope to that returned | ||
| * value if the module is loaded in a browser. | ||
| * - .importAs: the name that this chunk's exports object will be | ||
| * given when passed to the factory function of other chunks that | ||
| * depend on it. (Needs to be distinct from .exports since (e.g.) | ||
| * "Blockly.blocks.all" is not a valid variable name.) | ||
| * - .factoryPreamble: code to override the default wrapper factory | ||
| * function preamble. | ||
| * - .factoryPostamble: code to override the default wrapper factory | ||
| * function postabmle. | ||
| * | ||
| * The function getChunkOptions will, after running | ||
| * closure-calculate-chunks, update each chunk to add the following | ||
|
|
@@ -81,37 +99,49 @@ const chunks = [ | |
| { | ||
| name: 'blockly', | ||
| entry: 'core/requires.js', | ||
| namespace: 'Blockly', | ||
| wrapperSetup: `const ${NAMESPACE_OBJECT}={};`, | ||
| wrapperCleanup: | ||
| exports: 'Blockly', | ||
| importAs: 'Blockly', | ||
| factoryPreamble: `const ${NAMESPACE_OBJECT}={};`, | ||
| factoryPostamble: | ||
| `${NAMESPACE_OBJECT}.Blockly.internal_=${NAMESPACE_OBJECT};`, | ||
| }, { | ||
| name: 'blocks', | ||
| entry: 'blocks/all.js', | ||
| namespace: 'Blockly.blocks', | ||
| exports: 'Blockly.Blocks', | ||
| importAs: 'BlocklyBlocks', | ||
| }, { | ||
| name: 'javascript', | ||
| entry: 'generators/javascript/all.js', | ||
| namespace: 'Blockly.JavaScript', | ||
| exports: 'Blockly.JavaScript', | ||
| }, { | ||
| name: 'python', | ||
| entry: 'generators/python/all.js', | ||
| namespace: 'Blockly.Python', | ||
| exports: 'Blockly.Python', | ||
| }, { | ||
| name: 'php', | ||
| entry: 'generators/php/all.js', | ||
| namespace: 'Blockly.PHP', | ||
| exports: 'Blockly.PHP', | ||
| }, { | ||
| name: 'lua', | ||
| entry: 'generators/lua/all.js', | ||
| namespace: 'Blockly.Lua', | ||
| exports: 'Blockly.Lua', | ||
| }, { | ||
| name: 'dart', | ||
| entry: 'generators/dart/all.js', | ||
| namespace: 'Blockly.Dart', | ||
| exports: 'Blockly.Dart', | ||
| } | ||
| ]; | ||
|
|
||
| /** | ||
| * The default factory function premable. | ||
| */ | ||
| const FACTORY_PREAMBLE = `const ${NAMESPACE_OBJECT}=Blockly.internal_;`; | ||
|
|
||
| /** | ||
| * The default factory function postamble. | ||
| */ | ||
| const FACTORY_POSTAMBLE = ''; | ||
|
|
||
| const licenseRegex = `\\/\\*\\* | ||
| \\* @license | ||
| \\* (Copyright \\d+ (Google LLC|Massachusetts Institute of Technology)) | ||
|
|
@@ -283,8 +313,8 @@ function chunkWrapper(chunk) { | |
| const amdDeps = fileNames.join(', '); | ||
| const cjsDeps = fileNames.map(f => `require(${f})`).join(', '); | ||
| const browserDeps = | ||
| chunk.dependencies.map(d => `root.${d.namespace}`).join(', '); | ||
| const imports = chunk.dependencies.map(d => d.namespace).join(', '); | ||
| chunk.dependencies.map(d => `root.${d.exports}`).join(', '); | ||
| const imports = chunk.dependencies.map(d => d.importAs).join(', '); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every chunk previously had a
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only chunks which are depended upon by other chunks need an
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Arguably I should add a robustness check here, such that if it discovers a chunk that has become a dependancy but has no
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Then this works fine here, but it's fragile.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll file a low priority bug after I finish this review
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading more: I expect that this code will continue to change into next quarter, particularly when we mess around with entry points, so I'll hold off on that bug. |
||
| return `// Do not edit this file; automatically generated. | ||
|
|
||
| /* eslint-disable */ | ||
|
|
@@ -294,13 +324,13 @@ function chunkWrapper(chunk) { | |
| } else if (typeof exports === 'object') { // Node.js | ||
| module.exports = factory(${cjsDeps}); | ||
| } else { // Browser | ||
| root.${chunk.namespace} = factory(${browserDeps}); | ||
| root.${chunk.exports} = factory(${browserDeps}); | ||
| } | ||
| }(this, function(${imports}) { | ||
| ${chunk.wrapperSetup || `const ${NAMESPACE_OBJECT}=Blockly.internal_;`} | ||
| ${chunk.factoryPreamble || FACTORY_PREAMBLE} | ||
| %output% | ||
| ${chunk.wrapperCleanup || ''} | ||
| return ${NAMESPACE_OBJECT}.${chunk.namespace}; | ||
| ${chunk.factoryPostamble || FACTORY_POSTAMBLE} | ||
| return ${NAMESPACE_OBJECT}.${chunk.exports}; | ||
| })); | ||
| `; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,8 +126,8 @@ function packageBlockly() { | |
| */ | ||
| function packageBlocks() { | ||
| return gulp.src('scripts/package/blocks.js') | ||
| .pipe(packageUMD('Blockly.Blocks', [{ | ||
| name: 'Blockly', | ||
| .pipe(packageUMD('BlocklyBlocks', [{ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you keep the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mainly to try to not break anything, TBH, but yes: the documentation in the resulting files in I eventually think most of this should go away:
But in the mean time I figured that feeding an effectively-empty file to the existing pipeline was the safest thing to do for the time being. |
||
| name: 'BlocklyBlocks', | ||
| amd: './blocks_compressed', | ||
| cjs: './blocks_compressed', | ||
| }])) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL postamble is a word. Thanks!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had to check that myself. I was very glad to discover that it is.