-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: remove vestigial dependency #6219
Conversation
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.
This should be fine:
- The AMD and CJS wrapper entry points (and the body of the wrapped message files) do not depend on
Blockly
so have no need torequire
it. - While the browser entry point does depends on
Blockly
, it expect it to have been pre-loaded via<script>
, so removing it here has no effect on the generated stanza.
I am interested in in what the problem you encountered in blockly-samples was, though. Was it somehow caused by #6210? It looks to me like the real issue here is that 'Blockly'
is not the name of a file that can be loaded, that changing it to './blockly_compressed.js'
(or one of the wrappers thereof) would have worked equally well (albeit being, as noted above, unnecessary).
Oh yeah whoops, forgot to post. I was getting this error when running
I'm honestly not sure. I tried to manually change the message wrappers to use However, Johnny just got a similar error, which was not happening before. Which makes me think it was working before. But also, he's making a React app, so the AMD section of the wrapper shouldn't be affecting what he's doing. So I want to dig into that more before merging this. |
Sensible enough, but just to be clear: I think this PR is fine on its own merits and can be merged whenever you like; I just think it's worth understanding what was broken (and for how long, and if for long how we avoided noticing until now) for the purposes of avoiding the same mistake(s) in future. |
So I checked out the v8.0.0 tag and before the amd definition would be I tested manually adding this fix ( So I'm cool putting this in now. |
React probably (depends on something that)* depends on an AMD loader like RequireJS. |
(cherry picked from commit 334956b)
The basics
npm run format
andnpm run lint
The details
Resolves
Hopefully fixes problems with message dependencies we ran into in samples.
Proposed Changes
Removes the vestigially Blockly dependency from the message wrappers.
Behavior Before Change
The Blockly module could not be resolved.
Behavior After Change
No need to resolve any modules hopefully.
Reason for Changes
Getting the messages to actually work.
Test Coverage
I tested manually removing the dependencies in the node_modules of some plugins in samples, and that seemed to fix things.
I also tried linking to
field-bitmap
and runningnpm run predeploy
before and after this change, and the changes in this PR seemed to fix the problem.Documentation
Additional Information