Unable to import plugin-workspace-search 8.0.4 normally.

[simplenty]

Category

• Plugins

Component

• plugin-workspace-search

Describe the bug

In version 8.0.4, the library cannot be obtained normally through the package manager.

To Reproduce

import { WorkspaceSearch } from '@blockly/plugin-workspace-search'

Expected behavior

In the newly released version 8.0.4, when importing the plugin, an error will be reported that the library does not exist.

Screenshots

Additional context

In node_modules/@blockly/plugin-workspace-search/package.json, lines 14 to 16, there is:

  "main": "./dist/index.js",
  "module": "./src/index.js",
  "unpkg": "./dist/index.js",

If changed to the following:

  "main": "./dist/index.js",
  "module": "./dist/index.js",
  "unpkg": "./dist/index.js",

Then it can be used normally again.

[maribethb]

@cpcallen I think I need your help with this one.

For background, all of the plugins specify the module property to point to src/ instead of dist, and they have for a long time. This plugin was recently converted to TS which probably exposed this behavior, but I'm not sure why nobody has ever reported this error before (with other TS plugins?) and why we don't see it ourselves when building plugins.

It seems like we are using the module keyword incorrectly in package.json. According to this webpack guide, it should point to an ecmascript module, but the file doesn't exist in ts plugins. In js plugins, it's unclear to me whether the file is actually an esmodule (it uses esmodule-style import as opposed to commonjs require but we don't specify the file extensions in relative imports which I thought was a requirement).

It's also worth noting that the module keyword was introduced and supported in webpack because it was expected to be a proposal accepted in Node.js, but node went a different direction and uses type: "module" keyword alongside exports to determine which files should be interpreted as esmodules vs commonjs modules. So the module keyword is not actually supported by node, but does happen to still be supported by webpack and other build tools.

My best guess for what's happening is that the original reporter is using a build tool (Vite, which I know nothing about) that is more strict about the module actually existing.

So I still have a bunch of unknowns, including:

• Why did we add this to begin with? What problem did it solve?
• Did it ever work correctly? Does it work for the non-TS plugins where the file actually exists?
• Why doesn't webpack complain that the file doesn't exist for ts plugins?
• Why doesn't webpack complain that the file isn't actually an esmodule for js plugins (or that the relative import doesn't include the file extension)?
• Why doesn't Vite complain if the module points to dist/index.js which is definitely a umd module and not an esmodule?
• Is webpack's treeshaking different if we specify this module file? some reading suggests yes, but not really sure how this interacts with the alternative type keyword that node actually adopted.
  • It seems like webpack can perform better if its inputs are esmodule instead of commonjs. we can do that with the module keyword. if we want to do it with conditional exports then it seems like we'd have to actually publish the module as an esmodule which we aren't currently doing (there is no esmodule in the dist/ directory) and runs into the pitfalls described here

What I think we should do:

• Remove the module: "./src/index.js", from at minimum all plugins written in typescript.
  • consider removing it from all package.json files, even the js plugins where the file at least exists
  • I did this and all plugins build successfully... would rather do a bit more testing to make sure the compiled output is the same.
  • I tested the typed-variable-modal plugin which has a dependency on the modal plugin, just to make sure since the original issue says it was fixing something about local dependencies. Everything appears to be in good shape.
• We don't need to add the type property to package.json because the default is commonjs. We actually publish umd modules which are compatible with commonjs so this is adequate.
• Follow up to find out if we can publish an esmodule so that webpack can use that instead of the cjs module, for improved treeshaking performance.

@cpcallen does any of this sound plausible/reasonable? Any ideas on my unknowns?

[rachel-fenichel]

@technology-me did this work for you in v8.0.3?

[cpcallen]

[Edited to elaborate on earlier version:]

Prior to PR #1857, this plugin was configured as a dual ESM + CJS (actually UMD) package:

• dist/index.js is the usual CJS (actually UMD) version of the package.
• src/index.js was part of the source code, but also proffered as the ESM version of the package.
  • Given the existence of this issue it was evidently being ingested by some build systems one tried to import (rather than require()) the package.
  • It was actually a trivial wrapper around src/WorkspaceSearch.js.
  • Due to merely doing export * from './WorkspaceSearch'—omitting the .js suffix—it is also one that can only be used by build pipelines. A developer wishing to directly import this plugin from HTML would have need to do import … from path/to/plugin-workspace-search/src/WorkspaceSearch.js (rather than …/index.js`).

When we migrated the module to TS we broke this because src/index.js got renamed to src/index.ts so there is indeed no longer a src/index.js. There is no viable replacement candidate at present:

• dist/index.js is the UMD module, and although apparently @technology-me's build pipeline is OK with accepting a CJS module when it was expecting an ESM that is both a gross violation of the intent of the "module": directive and not safe to assume will generally be the case.
• src/workspace_serach.ts is not an ES module, because it's not written in ECMAScript.
• src/index.ts is a valid ES module (because it is so trivial) but importing it will fail because workspace_search.ts isn't.

I see a number of ways forward:

1. Configure tsc (or otherwise arrange) to put the compiled (.js) versions of the individual source files in a place where they will get included in the package, and update package.json so that the "module": directive points at them.
2. Configure tsc and/or webpack to—in addition to the existing UMD version—also build a single combined ESM, and put this in dist/ along with the CJS version.
3. Build only the CJS version as we do at present, but also include an ESM wrapper which will load the CJS version and export it.
4. As in option 1, but stop using webpack entirely and publish only the individual-ESM version.
5. As in option 2, but reconfigure webpack so as to publish only the bundled-ESM version.

For plugins that are still written in JavaScript, there is an additional option: only publish the source code and do away with the build pipeline entirely. (This is option 4, minus tsc.)

We should probably consider this decision in the context of google/blockly#7449, though we might make different choices (for example, an ESM-loads-CJS wrapper might make sense for Blockly proper, but having one per plugin would entail either adding a common (non-dev) dependency to all the plugins, or a lot of duplication of wrapper code between (the built versions of) packages.

[cpcallen]

Re option 4, publishing only individual .js files rather than a minified bundle:

• Most developers who are using plugins and care about loading speed will already have a build pipeline.

• It's more likely than not that their build pipeline will get a better overall result (smaller bundle) ingesting individual .js files than a minified bundle; tree shaking in particular seems more likely to work well—though this will depends on which build system is used.

• For small plugins (which is most of them), the space savings of bundling+minifcation are not particularly impressive. Consider for this plugin:

  workspace-search$ ls -lR src build/src dist
  src:
  -rw-r--r--  1 cpcallen  primarygroup   3071  4 Sep 13:17 css.ts
  -rw-r--r--  1 cpcallen  primarygroup    125  4 Sep 13:17 index.ts
  -rw-r--r--  1 cpcallen  primarygroup  17148  4 Sep 13:17 workspace_search.ts
  
  build/src:
  -rw-r--r--  1 cpcallen  primarygroup   3136  6 Sep 15:55 css.js
  -rw-r--r--  1 cpcallen  primarygroup    998  6 Sep 15:55 css.js.map
  -rw-r--r--  1 cpcallen  primarygroup    157  6 Sep 15:55 index.js
  -rw-r--r--  1 cpcallen  primarygroup    139  6 Sep 15:55 index.js.map
  -rw-r--r--  1 cpcallen  primarygroup  17908  6 Sep 15:55 workspace_search.js
  -rw-r--r--  1 cpcallen  primarygroup  11292  6 Sep 15:55 workspace_search.js.map
  
  dist:
  -rw-r--r--  1 cpcallen  primarygroup   9044  4 Sep 13:18 index.js
  -rw-r--r--  1 cpcallen  primarygroup     88  4 Sep 13:18 index.js.LICENSE.txt
  -rw-r--r--  1 cpcallen  primarygroup  32322  4 Sep 13:18 index.js.map
  

  Minification saves only about 50% compared to the individual .js files (and increases the size of the sourcemap by 260%!)

But on the other hand:

• Loading several modules will be slower than loading just one for developers doing local testing (or, worse, shipping) without building.

[cpcallen]

Separately, we should consider getting rid of trivial wrappers like index.js, as they serve (AFAICT) no purpose.

[simplenty]

Yes, I can still use v8.0.3 normally.

[alicialics]

I've attempted to build this plugin using esbuild (basically option 5 but without webpack) and it results in 10% smaller bundle size for cjs format.

-rw-r--r--  1    8775 Sep 26 23:25 index.cjs.js
-rw-r--r--  1   27993 Sep 26 23:25 index.cjs.js.map
-rw-r--r--  1    8283 Sep 26 23:25 index.d.mts
-rw-r--r--  1    8283 Sep 26 23:25 index.d.ts
-rw-r--r--  1    8149 Sep 26 23:25 index.esm.js
-rw-r--r--  1   27721 Sep 26 23:25 index.esm.js.map
-rw-r--r--  1   19436 Sep 26 23:27 index.js

for the current webpack setup:

-rw-r--r--  1   10094 Sep 26 23:34 index.js
-rw-r--r--  1      88 Sep 26 23:34 index.js.LICENSE.txt
-rw-r--r--  1   31902 Sep 26 23:34 index.js.map

[cpcallen]

I propose that we fix this temporarily by removing the "module": "./src/index.js", line from the package.json file of all the plugins implemented in TypeScript—i.e., not advertise ESM source files that do not in fact exist—until we have resolved the question about how (or indeed whether) we want to publish dual ESM/CJS packages. That should resolve build problems like this one, at least for developers whose bundler is clever enough to use ESM/CJS modules interchangeably, which I think is most of them.

We could make the same change to JS-soureced packages for consistency, but I don't see any reason to meddle since AFAIK there is nothing broken there that needs fixing.