Skip to content
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: multiple blockly instances #2375

Merged
merged 4 commits into from
May 24, 2024

Conversation

BeksOmega
Copy link
Contributor

@BeksOmega BeksOmega commented May 24, 2024

The basics

The details

Resolves

Fixes #2361

Proposed Changes

dev-dependencies

We need a new way to make sure we don't end up with multiple instances of the Blockly package in one plugin. There are multiple ways to do this (including ones that are more robust like using NPM workspaces), but this was the most efficient way to get the issue resolved quickly.

See #2361 (comment) for info about how we fixed this before using resolve.alias.

What we do now is we make Blockly a dev-dependency of the root, and only a peer-dependency of the individual plugins.

Being a peer-dependency seems to mean that when the dependency is installed, non if its depedencies get installed. This is different from dev-depedencies, where dependencies do get installed recursively. But this is all pretty unclear because people on the internet have argued over what the behavior /should/ be... quite a lot.

Now that doesn't solve our problem.

We also need to use legacy-peer-deps. Before npm v7 peer deps wouldn't be installed /at all/. They were just assumed to be provided. That is the behavior that we want. Every plugin assumes that Blockly is depended on by the package that depends on it (or some parent of that package), so it never installs Blockly. This means we only get one instance of Blockly in the root of the monorepo.

tsconfigs paths

So we originally had paths in the tsconfigs files because of this: #1934 We wanted to make sure that when typescript was resolving types, it was using its version of a given plugin.

Note that this does not affect the paths that are output so even if something correctly resolves for typescript, it may not correctly resolve for other levels of compilation (e.g. webpack) or at runtime. Additionally, when you're resolving something from a "path" package.json "exports" are ignored.

While module specifiers that match paths aliases are bare specifiers, once the alias is resolved, module resolution proceeds on the resolved path as a relative path. Consequently, resolution features that happen for node_modules package lookups, including package.json "exports" field support, do not take effect when a paths alias is matched. This can lead to surprising behavior if paths is used to point to a node_modules package:

https://www.typescriptlang.org/docs/handbook/modules/reference.html#paths:~:text=While%20module%20specifiers,a%20node_modules%20package%3A

This is why we didn't previously get the errors in the shadow-block-convert plugin, because things were fine from Typescript's perspective, even though they weren't actually fine.

Now the question is why it didn't blow up at as when the "paths" option stopped resolving. Based on logging from tsc --traceResolution, if your "paths" doesn't resolve it falls back to normal "bundler" resolution, which essentially searches for any ts or d.ts in any node_modules folder.

======== Resolving module 'blockly/core' from '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter/src/shadow_block_converter.ts'. ========
Explicitly specified module resolution kind: 'Bundler'.
Resolving in CJS mode with conditions 'import', 'types'.
'baseUrl' option is set to '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter', using this value to resolve non-relative module name 'blockly/core'.
'paths' option is specified, looking for a pattern to match module name 'blockly/core'.
Module name 'blockly/core', matched pattern 'blockly/*'.
Trying substitution 'node_modules/blockly/*', candidate module location: 'node_modules/blockly/core'.
Loading module as file / folder, candidate module location '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter/node_modules/blockly/core', target file types: TypeScript, JavaScript, Declaration, JSON.
File '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter/src/package.json' does not exist.
Found 'package.json' at '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter/package.json'.
Loading module 'blockly/core' from 'node_modules' folder, target file types: TypeScript, JavaScript, Declaration, JSON.
Searching all ancestor node_modules directories for preferred extensions: TypeScript, Declaration.
Directory '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter/src/node_modules' does not exist, skipping all lookups in it.
Directory '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/shadow-block-converter/node_modules/@types' does not exist, skipping all lookups in it.
Directory '/usr/local/google/home/bwestberg/Dev/blockly-samples/plugins/node_modules' does not exist, skipping all lookups in it.
File '/usr/local/google/home/bwestberg/Dev/blockly-samples/node_modules/blockly/core/package.json' does not exist.
Found 'package.json' at '/usr/local/google/home/bwestberg/Dev/blockly-samples/node_modules/blockly/package.json'.
Entering conditional exports.
Matched 'exports' condition 'types'.
Using 'exports' subpath './core' with target './core.d.ts'.
File '/usr/local/google/home/bwestberg/Dev/blockly-samples/node_modules/blockly/core.d.ts' exists - use it as a name resolution result.
Resolved under condition 'types'.
Exiting conditional exports.
Resolving real path for '/usr/local/google/home/bwestberg/Dev/blockly-samples/node_modules/blockly/core.d.ts', result '/usr/local/google/home/bwestberg/Dev/blockly-samples/node_modules/blockly/core.d.ts'.
======== Module name 'blockly/core' was successfully resolved to '/usr/local/google/home/bwestberg/Dev/blockly-samples/node_modules/blockly/core.d.ts' with Package ID 'blockly/[email protected]'. ========

It wasn't telling us our thing was broken because it falls back and tries to read our mind. It is possible this is because webpack, rollup, and other bundlers similarly behave this way. But I'm not 100% sure they do, and I'm not 100% sure that would be why.

In any case, we no longer need the "paths" since we only want to have one version of Blockly in the repo (in the root node_modules) and the normal resolution system works just fine for that.

Test Coverage

Tested that the "Random Blocks" action works now.

Additional Info

Because we only have one version of Blockly in the repo now (ignoring examples), whenever we bump the peer dependency of a plugin, we need to bump the dev dependency in the root package.json. E.g. if we bump a plugin from ^11.x.0 to ^11.x+1.0 we need to bump the dev dependency to ^11.x+1.0 otherwise the dev dependency will be incompatible with the plugin.

@BeksOmega BeksOmega requested a review from maribethb May 24, 2024 18:41
@BeksOmega BeksOmega requested a review from a team as a code owner May 24, 2024 18:41
@BeksOmega BeksOmega merged commit b231e59 into google:master May 24, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants