fix(dev-scripts)!: Fixes, refactoring and simplification of webpack.config.js and 'blockly' imports
#2229
Conversation
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
changed the title
webpack.config.js and 'blockly' importswebpack.config.js and 'blockly' imports
|
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. |
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.
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.
|
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 Therefore I think you have two options:
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 |
That commit is already not a breaking change. Are you thinking of |
|
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. |
The basics
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 aresolve.aliasforblocklytowebpack.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'spackage.jsonfile. 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)!: Introduceexportssection inpackage.jsonblockly#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 theexternalsstanza ofwebpack.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
jsdomsubdependencies: addbufferutilsandutf-8-validateto theIgnorePluginconfig when building tests. These are optional dependencies of wd, which is itself a dependency of jsdom. Also refactor how thepluginsconfig is generated to improve readability.Simplify
resolve.extensions. There doesn't appear to be any harm in including'.ts'inresolve.extensionseven 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]sfiles 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)!: Introduceexportssection inpackage.jsonblockly#7822). Fortunately the'blockly'entrypoint behaves the same as the'blockly/node'entrypoint does in a node.js environment.Reason for Changes
webpack.config.js.Test Coverage
The
build,testandstartscripts 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.0branch.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-samplesthat 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 inwebpack.config.jsdepended on apostinstallscript that was removed in PR #1630.When testing such PRs going forward use
npm linkfor local testing and wait for changes to blockly to be publishedbefore 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!