fix(dev-scripts)!: Fixes, refactoring and simplification of `webpack.config.js` and `'blockly'` imports #2228

cpcallen · 2024-02-28T19:17:54Z

The basics

I validated my changes

The details

Resolves

Part of google/blockly#7449.

Proposed Changes

Remove support acquiring Blockly through git. This completes the revert of PR Support acquiring Blockly through Git instead of npm #335. See BREAKING CHANGE below for more details.
Don't use alias when resolving blockly. PR Standardize build and start config #226 added a resolve.alias for blockly to webpack.config.js. It is not entirely clear what the purpose of this was at the time, but it has the effect of treating imports of submodules (e.g. 'blockly/core') as if they were direct imports (e.g. of ./node_modules/blockly/core.js, causing webpack to ignore the blockly package's package.json file. This causes plugins to fail to build due to the introduction of an exports stanza in that file (and other related changes) in feat(build)!: Introduce exports section in package.json blockly#7822.
Exclude Blockly from plugin bundles. This fixes bloat caused by some plugins importing all of 'blockly' (instead of just 'blockly/core'), resulting in webpack including a copy of blockly in the bundled plugin because only the subpackage entrypoints were listed in the externals stanza of webpack.config.js. This will also avoid certain problems that might occur due to apps using such bundles inadvertently containing two or more different copies of Blockly.
Also fix the one plugin which did still have an unnecessary dependency on 'blockly' intead of 'blockly/core'.
Introduce an exists() function for readability.
Ignore more jsdom subdependencies: add bufferutils and utf-8-validate to the IgnorePlugin config when building tests. These are optional dependencies of wd, which is itself a dependency of jsdom. Also refactor how the plugins config is generated to improve readability.
Simplify resolve.extensions. There doesn't appear to be any harm in including '.ts' in resolve.extensions even for pure-JS plugins, but it is necessary to include it for TS plugins. Since the default value for resolve.extensions is
['.js', '.json', '.wasm']
set it to
['.ts', '.js', '.json', '.wasm']
which gives priority to TS, then JS, then the other default extensions.
Add various comments to help future readers.
Update several plugin's tests/*.mocha.[jt]s files to import 'blockly', not 'blockly/node' The latter has never been an advertised entrypoint, and will cease to be a valid entrypoint in v11 (see feat(build)!: Introduce exports section in package.json blockly#7822). Fortunately the 'blockly' entrypoint behaves the same as the 'blockly/node' entrypoint does in a node.js environment.

Reason for Changes

Ensure plugins will continue to build after google/blocky#7822 is merged and published.
Improve readability of webpack.config.js.

Test Coverage

The build, test and start scripts for each plugin were tested against both Blockly v10.4.2 and Blockly v11.0.0.beta-3 + google/blockly#7822.

Additional Information

BREAKING CHANGE: This PR removes the support that was added in PR #335 for acquiring Blockly directly from a git:// URL.

This feature was useful insofar as it enabled merging changes into blockly-samples that depend on changes in blockly that have not yet been published (even as a beta)—and still have tests pass. For this to work seamlessly, however, the code in webpack.config.js depended on a postinstall script that was removed in PR #1630.

When testing such PRs going forward use npm link for local testing and wait for changes to blockly to be published
before merging the corresponding changes to blockly-samples—or wait for blockly to become a monorepo so both changes can be made in the same PR!

Completes revert of PR google#335. BREAKING CHANGE: This PR removes the support that was added in PR google#335 for aquiring Blockly directly from a git:// URL. This feature was useful insofar as it enabled merging changes into blockly-samples that depend on changes in blockly that have not yet been published (even as a beta)—and still have tests pass. For this to work seamlessly, however, the code in webpack.config.js depended on a postinstall script that was removed in PR google#1630. When testing such PRs going forward use npm link for local testing and wait for changes to blockly to be published before merging the corresponding changes to samples—or wait for blockly to become a monorepo so both changes can be made in the same PR! Note that this change is breaking only to the dev-scripts plugin itself, and will not cause other plugins that use it as a dev dependency to experience a breaking change.

The commit which removed support for git:// URLS by completing the revert of PR google#335 removed the initialisation of packageJson (from the package.json of the plugin being built) which turns out to still be needed by a DefinePlugin call added later.

PR google#226 addedd a resolve.alias for blockly to webpack.config.js. It is not entirely clear what the purpose of this was at the time, but it has the effect of treating imports of submodules (e.g. 'blockly/core') as if they were direct imports (e.g. of './node_modules/blockly/core.js'), causing webpack to ignore the blockly package's package.json file. This causes plugins to fail to build due to the introduction of an exports stanza in that file (and other related changes) in google/blockly#7822.

Fix bloat caused by some plugins depending on all of blockly (instead of just blockly/core), resulting in webpack including a copy of blockly in the bundled plugin becuase only the subpackage entrypoints were listed inthe externals stanza of webpack.config.js. This will also avoid certain problems that might occur due to apps using such bundled inadvertently containing two or more different copies of Blockly. Also fix the one plugin which did still have an unnecessary dependency on blockly intead of blockly/core.

Currently webpack.conf.js is hard to understand. Attempt to improve readability by making some parts more DRY ("don't repeat yourself") and others more DAMP ("descriptive and meaningful phrases").

Add bufferutils and utf-8-validate to IgnorePlugin config when building tests. These are optional dependencies of wd, which is itself a dependency of jsdom. Also refactor how plugins config is generated to improve readability.

There doesn't appear to be any reason not to include the '.ts' in resolve.extensions even for pure-JS plugins, but it is _necessary_ to include it in TS plugins. Since the default value for resolve.extensions is ['.js', '.json', '.wasm'] set it to ['.ts', '.js', '.json', '.wasm'] which gives priority to TS, then JS, then the other default extensions. Also add a helpful comment explaining the purpose of resolve.fallback.

The latter has never been an advertised entrypoint, and will cease to be a valid entrypoint in v11 (see google/blockly#7822). Fortunately the 'blockly' entrypoint behaves the same as the 'blockly/node' entrypoint does in a node.js environment.

cpcallen · 2024-02-28T19:18:47Z

Note to reviewer: it may be helpful to review this PR commit-by-commit.

maribethb · 2024-02-28T23:43:20Z

I haven't reviewed this yet, but I will note that doing a breaking change in dev-tools/dev-scripts will cause every other plugin to also get a major version bump. Since we don't "need" this until blockly v11, which will independently also cause a major version bump in every plugin, I would prefer to bundle those together and therefore submit this to the v11-specific branch in samples.

cpcallen · 2024-02-29T10:24:51Z

[Edited for clarity.]

I will note that doing a breaking change in dev-tools/dev-scripts will cause every other plugin to also get a major version bump.

That is surprising. This change to dev-scripts should not have any effect on the contents (and thus the backward-compatibility and major version number) of any of the published plugin packages, since currently none of them depends on a git:// URL.

In the general case, while it's possible that any change to any kind of dependency could result in a breaking change to the dependent package, it's not automatic, and breaking changes to dev dependencies are even less likely to cause one than breaking changes to non-dev dependencies.

Is this a result of our lerna config / publish scripts?

cpcallen · 2024-03-05T18:30:34Z

[Comment moved from #2229 as it looks like this one will probably be the one that gets merged.]

CI is failing here and in #2229 due to multiple similar test failures in plugins/block-dynamic-connection, e.g.:

  1) If block
       Serialization
         append block
           one if one else - old serialization:
     TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
      at exports.convert (webpack-internal:///../dev-tools/node_modules/jsdom/lib/jsdom/living/generated/Node.js:25:9)
      at Element.appendChild (webpack-internal:///../dev-tools/node_modules/jsdom/lib/jsdom/living/generated/Node.js:404:26)
      at blockToDom$$module$build$src$core$xml (webpack-internal:///../dev-tools/node_modules/blockly/blockly_compressed.js:163:393)
      at blockToDomWithXY$$module$build$src$core$xml (webpack-internal:///../dev-tools/node_modules/blockly/blockly_compressed.js:161:195)
      at new BlockCreate$$module$build$src$core$events$events_block_create (webpack-internal:///../dev-tools/node_modules/blockly/blockly_compressed.js:610:462)
      at domToBlockInternal$$module$build$src$core$xml (webpack-internal:///../dev-tools/node_modules/blockly/blockly_compressed.js:180:405)
      at $.domToBlock$$module$build$src$core$xml [as domToBlock] (webpack-internal:///../dev-tools/node_modules/blockly/blockly_compressed.js:178:81)
      at Context.eval (webpack-internal:///../dev-tools/src/block_test_helpers.mocha.js:191:63)

These errors have the same root cause the test failures in #1452: multiple copies of jsdom.

In that case the issue was that each of the tests/*.mocha.js files get webpacked with their own separate copy of blockly + jsdom, and due to how Blockly` handled polyfilling XML DOM functions on Node this resulted in some of the copies of Blockly not being able to do (de)serialisation.

In this case the issue appears to be that, due to the removal of the alias clause from webpack.config.js, each of the tests/*.mocha.js bundles ends up getting at least two separate copies of blockly + jsdom: one from plugins/dev-tools/node_modules/ and one from plugins/block-dynamic-connection/node_modules/. This then results in XML DOM objects created by runSerializationTestSuite (using it's copy of jsdom) failing type checks done by the copy of jsdom used by the instance of blockly that is smuggled in to that runSerializationTestSuite via this.workspace.

Additional clarification:

In the previous case the multiple copies of jsdom were being created by a single copy of jsdom being ingested multiple times by webpack; in the current case the cause is multiple separate copies of jsdom in different node_modules directories. (There were multiple copies before, too, but the alias entry for blockly in webpack.config.js caused only one to be used.)
This problem applies to any blockly-samples plugin using runSerializationTestSuite; blockly-dynamic-connection is just the first of these to fail when running npm test at the top level.)
The problem probably only affects plugins in blockly-samples. Specifically, failure depends on the @blockly/dev-tools package being installed with a copy of blockly (and thus jsdom) in its node_modules directory, which should only happen if it is checked out for development rather than being installed as a package from the npmjs repository.

Possible fixes:

Pass Blockly into runSerializationTestSuite as an argument, instead of importing it.
Figure out how to ensure that there is only one copy of (blockly and) jsdom that is shared between the plugins.
- Using npm link to link each of the plugins to a local copy of blockly/dist achieves this, but is not a suitable solution for testing in CI at the present time. This is, however, why this failure was not noticed locally when this PR was being prepared.
- Such a change would seem like good hygiene, in any case, though it raises a question about what should happen if different plugins want to test with different versions of Blockly. Probably only a temporary concern, however, as in a monorepo we would always test against the current version of Blockly.
Figure out how to get webpack to include only one copy of jsdom in the bundle for each .mocha.js file.
- In Blockly v11 we can't alias blockly as this causes package.json to be ignored, but it might be possible to alias jsdom or find some other way to ensure that only one copy is loaded.
- Unfortunately this means that these tests become dependent on being build with webpack, since running them directly in node.js will still obey the normal node package resolution rules.
Fix Blockly so it no longer has global state.
Make jsdom an optional peer dependency of Blockly (I think…)
Remove jsdom as a dependency of Blockly and force node.js users to inject it manually. (This might create problems for node.js users who want to use packages that depend on Blockly which don't provide a mechanism to inject it there, however.)
Make Blockly's use of jsdom entirely via global variables, so all copies would share a single instance.

Have runSerializationTestSuite accept a second argument which is the Blockly object to use for XML serialization/deserialization. This works around an issue in blockly-samples that (following the deletion of the alias for blockly in webpack.config.js) causes tests using this function to fail due to having two copies of jsdom loaded, where each will reject DOM objects created by the other. See google#2229 (comment) for more details.

cpcallen · 2024-03-11T11:57:26Z

Even after modifying runSerializationTestSuite to work around the problem caused by multiple copies of jsdom being included in each test bundle, one additional test, in plugins/typed-variable-modal, is still failing:

  1) TypedVariableModal
       show()
         Elements focused:
     TypeError: Cannot read properties of undefined (reading 'appendChild')
      at TypedVariableModal.widgetCreate_ (webpack-internal:///../modal/src/index.js:159:15)
      at TypedVariableModal.show (webpack-internal:///../modal/src/index.js:117:10)
      at Context.eval (webpack-internal:///./test/typed_variable_modal_test.mocha.js:93:26)

The root cause is the same, but this time it's the fact that there are two copies of the blockly npm package included in the test bundle: one for typed-variable-modal and a second one for modal. The former is used for the Blockly.inject() call and has containerDiv (in core/widgetdiv.ts) set, while the latter is used when modal's Modal.prototoype.widgetCreate_ method calls Blockly.WidgetDiv.getDiv, which finds that containerDiv in that copy of Blockly is still undefined and throws the error seen above.

After discussing this with the core team it has been decided to "solve" this problem by disabling this particular test for the time being, since we expect that moving to the proposed new, npm-workspace-based arrangement will solve this problem (and also the one that necessitated the previous change to runSerializationTestSuite).

cpcallen · 2024-03-11T13:57:27Z

Ok, @maribethb: this and #2229 (same changes, but on v11 branch) are now ready for review.

cpcallen added 8 commits February 24, 2024 12:48

refactor(dev-scripts): Introduce exists() for readability

6cc5056

Currently webpack.conf.js is hard to understand. Attempt to improve readability by making some parts more DRY ("don't repeat yourself") and others more DAMP ("descriptive and meaningful phrases").

fix(dev-scripts): Ignore more jsdom subdependencies

717d612

Add bufferutils and utf-8-validate to IgnorePlugin config when building tests. These are optional dependencies of wd, which is itself a dependency of jsdom. Also refactor how plugins config is generated to improve readability.

cpcallen added the category: plugin Anything in the plugins folder label Feb 28, 2024

cpcallen assigned maribethb Feb 28, 2024

cpcallen requested a review from a team as a code owner February 28, 2024 19:17

cpcallen requested review from BeksOmega and removed request for a team February 28, 2024 19:17

github-actions bot assigned BeksOmega Feb 28, 2024

cpcallen requested review from maribethb and removed request for BeksOmega February 28, 2024 19:19

cpcallen unassigned BeksOmega Feb 28, 2024

chore(plugins): Fix lint, formatting

76d3ce8

cpcallen mentioned this pull request Feb 29, 2024

fix(dev-scripts)!: Fixes, refactoring and simplification of webpack.config.js and 'blockly' imports #2229

Merged

1 task

cpcallen changed the title ~~fix!(dev-scripts): Fixes, refactoring and simplification of webpack.config.js and 'blockly' imports~~ fix(dev-scripts)!: Fixes, refactoring and simplification of webpack.config.js and 'blockly' imports Feb 29, 2024

chore(typed-variable-modal): Skip failing test

408854e

cpcallen closed this Mar 15, 2024

cpcallen mentioned this pull request May 17, 2024

Multiple Blockly instances due to removal of resolve.alias (as well as historical structure of our monorepo...) #2361

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dev-scripts)!: Fixes, refactoring and simplification of `webpack.config.js` and `'blockly'` imports #2228

fix(dev-scripts)!: Fixes, refactoring and simplification of `webpack.config.js` and `'blockly'` imports #2228

cpcallen commented Feb 28, 2024 •

edited

Loading

cpcallen commented Feb 28, 2024

maribethb commented Feb 28, 2024 •

edited

Loading

cpcallen commented Feb 29, 2024 •

edited

Loading

cpcallen commented Mar 5, 2024 •

edited

Loading

cpcallen commented Mar 11, 2024

cpcallen commented Mar 11, 2024

fix(dev-scripts)!: Fixes, refactoring and simplification of webpack.config.js and 'blockly' imports #2228

fix(dev-scripts)!: Fixes, refactoring and simplification of webpack.config.js and 'blockly' imports #2228

Conversation

cpcallen commented Feb 28, 2024 • edited Loading

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

cpcallen commented Feb 28, 2024

maribethb commented Feb 28, 2024 • edited Loading

cpcallen commented Feb 29, 2024 • edited Loading

cpcallen commented Mar 5, 2024 • edited Loading

cpcallen commented Mar 11, 2024

cpcallen commented Mar 11, 2024

fix(dev-scripts)!: Fixes, refactoring and simplification of `webpack.config.js` and `'blockly'` imports #2228

fix(dev-scripts)!: Fixes, refactoring and simplification of `webpack.config.js` and `'blockly'` imports #2228

cpcallen commented Feb 28, 2024 •

edited

Loading

maribethb commented Feb 28, 2024 •

edited

Loading

cpcallen commented Feb 29, 2024 •

edited

Loading

cpcallen commented Mar 5, 2024 •

edited

Loading