feat: support wrangler 1.x module specifiers with a deprecation warning #596
Conversation
🦋 Changeset detectedLatest commit: 4728593 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/1985472671/npm-package-wrangler-596You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/596/npm-package-wrangler-596Or you can use npx https://prerelease-registry.developers.workers.dev/runs/1985472671/npm-package-wrangler-596 dev path/to/script.js |
threepointone
force-pushed
the
deprecate-legacy-module-specifiers
branch
from
1c872cd to
ff24697
Compare
threepointone
changed the title
threepointone
force-pushed
the
deprecate-legacy-module-specifiers
branch
2 times, most recently
from
4c03f34 to
1cdda06
Compare
threepointone
changed the title
This implements wrangler 1.x style module specifiers, but also logs a deprecation warning for every usage.
Consider a project like so:
```
project
├── index.js
└── some-dependency.js
```
where the content of `index.js` is:
```jsx
import SomeDependency from "some-dependency.js";
addEventListener("fetch", (event) => {
// ...
});
```
`wrangler` 1.x would resolve `import SomeDependency from "some-dependency.js";` to the file `some-dependency.js`. This will work in `wrangler` v2, but it will log a deprecation warning. Instead, you should rewrite the import to specify that it's a relative path, like so:
```diff
- import SomeDependency from "some-dependency.js";
+ import SomeDependency from "./some-dependency.js";
```
In a near future version, this will become a breaking deprecation and throw an error.
(This also updates `workers-chat-demo` to use the older style specifier, since that's how it currently is at https://github.com/cloudflare/workers-chat-demo)
Known issue: This might not work as expected with `.js`/`.cjs`/`.mjs` files as expected, but that's something to be fixed overall with the module system.
Closes #586
threepointone
force-pushed
the
deprecate-legacy-module-specifiers
branch
from
1cdda06 to
4728593
Compare
| { | ||
| filter: new RegExp( | ||
| [...props.wrangler1xlegacyModuleReferences.fileNames] | ||
| .map((fileName) => `^${fileName}$`) |
There was a problem hiding this comment.
Is it possible for deeper paths to be imported in this way in Wrangler 1?
e.g import HTML from "views/chat.html";
Is it possible for modules in subdirectories to use this kind of import? And if so, should the import be relative to the current module or to the root?
There was a problem hiding this comment.
Should this regex simply match any import that does not start with a . and then see if it can resolve it to a real file?
There was a problem hiding this comment.
- I checked, and it's not possible for deeper paths to be here. That was part of the quirks of wrangler 1. (and why you couldn't have wasm inside npm modules, and why there was a special webpack type, etc etc)
- I don't want to change the regex to be any non-
.file because then there's the whole possible clashes with node modules etc etc. But this should cover the usecases (happy to revisit if I'm wrong).
| external: props.format === "modules", // mark it as external in the bundle | ||
| namespace: `wrangler-module-${rule.type}`, // just a tag, this isn't strictly necessary | ||
| watchFiles: [filePath], // we also add the file to esbuild's watch list | ||
| }; |
There was a problem hiding this comment.
There is a lot of duplication here with the actual module-collection plugin handlers.
I appreciate that you are trying to keep this bit separate so that it is easy to remove later.
But I think there is still room for creating some helper function or two to remove that duplication.
There was a problem hiding this comment.
That's fair, and you're right I did try to keep it separate so we could just delete at one go. Lemme spend a little time tonight and try to function-ify some bits.
There was a problem hiding this comment.
honestly, it doesn't seem worth it. there's just enough differences interspersed in the body where the abstraction will be a weird argument signature, and it'll only get removed by 2.1. I'll give it a go when I try to implement bundle splitting, that might tease it apart some more.
There was a problem hiding this comment.
Cool. Thanks for taking a look.
This implements wrangler 1.x style module specifiers, but also logs a deprecation warning for every usage.
Consider a project like so:
where the content of
index.jsis:wrangler1.x would resolveimport SomeDependency from "some-dependency.js";to the filesome-dependency.js. This will work inwranglerv2, but it will log a deprecation warning. Instead, you should rewrite the import to specify that it's a relative path, like so:In a near future version, this will become a breaking deprecation and throw an error.
(This also updates
workers-chat-demoto use the older style specifier, since that's how it currently is at https://github.com/cloudflare/workers-chat-demo)Known issue: This might not work as expected with
.js/.cjs/.mjsfiles as expected, but that's something to be fixed overall with the module system.Closes #586