blockly-samples icon indicating copy to clipboard operation
blockly-samples copied to clipboard

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

Open cpcallen opened this issue 1 year ago • 1 comments

The basics

The details

Resolves

Part of google/blockly#7449.

Proposed Changes

  • Remove support acquiring Blockly through git. This completes the revert of PR #335. See BREAKING CHANGE below for more details.

  • Don't use alias when resolving blockly. PR #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 google/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 google/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

This is the same as #2228 but targeting the rc/v11.0.0 branch.

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!

cpcallen avatar Feb 29 '24 16:02 cpcallen

CI is failing here and on #2228; comment about this moved to #2228 as it looks like that PR will probably be the one that gets merged.

cpcallen avatar Mar 05 '24 16:03 cpcallen

So I think you're right that the breaking change to dev-scripts won't necessarily cause all the others to get a new major version. However, any plugin that is directly touched in this PR would get a new major version, because this PR is marked as breaking, and those plugins have changes in them. The fact that the changes are only in tests do not affect Lerna's versioning decisions. We could probably configure changes only in test/ to be ignored by lerna, but I think that's outside the scope of this PR and I'd have to think more about it. (edit: filed https://github.com/google/blockly-samples/issues/2240)

Therefore I think you have two options:

  1. Separate out fix(dev-tools): Have runSerializationTestSuite accept Blockly into a different, non-breaking PR, then merge #2228 with only changes to dev-scripts so that is the only plugin with breaking changes (if you pick this option, I'd want to run npm run publish:checkVersions after it's merged to confirm that not everything will get bumped, but I'm pretty sure you were right about that)
  2. Just merge this change wholesale into the v11 branch, all the affected plugins would get major version bumps, but they'll have to anyway when we release v11 so it doesn't matter.

I think option 2 is fine unless there's a good reason we need this now in master.

edit: there are a few other changes we'd also have to separate out for option 1, like changing from blockly to blockly/core in the one plugin

maribethb avatar Mar 14 '24 18:03 maribethb

  1. Separate out fix(dev-tools): Have runSerializationTestSuite accept Blockly into a different, non-breaking change…

That commit is already not a breaking change. Are you thinking of chore(dev-scripts)!: Remove support acquiring Blockly through git? That would difficult to separate out from the other webpack config changes.

cpcallen avatar Mar 14 '24 23:03 cpcallen

Any plugins that have any changes made in this PR will get new major versions because this PR is overall a breaking change.

maribethb avatar Mar 14 '24 23:03 maribethb

  1. So I think you're right that the breaking change to dev-scripts won't necessarily cause all the others to get a new major version. However, any plugin that is directly touched in this PR would get a new major version, because this PR is marked as breaking, and those plugins have changes in them.

  2. Any plugins that have any changes made in this PR will get new major versions because this PR is overall a breaking change.

You'd have thought I'd have actually read this the first time, but it didn't sink in until some time after I sent me reply. Sigh. Thanks for re-explaining the thing you'd already explained.

cpcallen avatar Mar 15 '24 21:03 cpcallen