fix: loading messages from script tags. #6184
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
cpcallen
requested changes
Jun 13, 2022
scripts/gulpfiles/package_tasks.js
Outdated
| @@ -29,12 +29,14 @@ const TEMPLATE_DIR = 'scripts/package/templates'; | |||
| * @param {string} namespace The export namespace. | |||
| * @param {Array<Object>} dependencies An array of dependencies to inject. | |||
| */ | |||
| function packageUMD(namespace, dependencies) { | |||
| function packageUMD( | |||
| namespace, dependencies, exportsName = namespace, template = 'umd.template' | |||
There was a problem hiding this comment.
As noted we can probably omit the exportsName parameter and just hardcode return messages' in msg-umd.template.
| } else if (typeof exports === 'object') { // Node.js | ||
| module.exports = factory(); | ||
| } else { // Browser | ||
| Object.assign(root.<%= namespace %>, factory()); |
There was a problem hiding this comment.
This will not be compatible with IE11. As discussed: it should be fine to use .setLocale here.
BeksOmega
commented
Jun 13, 2022
Comment on lines
-326
to
-329
| amd: '../core', | ||
| cjs: '../core', |
There was a problem hiding this comment.
Remove dependencies because messages don't actually depend on core.
cpcallen
approved these changes
Jun 13, 2022
scripts/gulpfiles/package_tasks.js
Outdated
Comment on lines
32
to
34
| function packageUMD( | ||
| namespace, dependencies, template = 'umd.template' | ||
| ) { |
Comment on lines
8
to
9
| const messages = factory(); | ||
| for (const key in messages) { |
There was a problem hiding this comment.
As noted: IE11 doesn't much like const, so use var.
BeksOmega
added a commit
to BeksOmega/blockly
that referenced
this pull request
Jun 13, 2022
* fix: loading messages in the browser * chore: fix comment * fix: change unwrapped message files to write to a new object, rather than Blockly.Msg * fix: fixup exports * fix: PR comments * fix: change to use for-in loop * fix: ES6 compatibility and formatting
Merged
4 tasks
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.
The basics
npm run formatandnpm run lintThe details
Resolves
Fixes #6123, fixes google/blockly-samples#1066 and fixes google/blockly-samples#1065
Same as #6060, but hopefully this also works for requiring multiple CJS modules.
Proposed Changes
See #6123 (comment)
Also changed the generated, but unwrapped, msg.js files to write to a new
messagesobject, rather thanBlockly.Msgto be clearer about what we're doing, and hopefully prevent errors in the future.Behavior Before Change
Message loading from script tags does not work.
Behavior After Change
Message loading from script tags works.
Reason for Changes
See #6123 (comment)
Test Coverage
prepare.jsto pull thedist/msg/es.jsfile instead ofmsg/messages.jsimport Fr from 'blockly/msg/fr'andBlockly.setLocale(Fr)I couldn't find anything that actually uses CJS modules to be able to test.
Documentation
N/A
Additional Information
Will undraft after testing.