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

πŸš€ Feature Request β€” Provide the raw-specifier in the Module fallback service requests #2112

Closed
dario-piotrowicz opened this issue May 10, 2024 · 2 comments Β· Fixed by #2131
Assignees
Labels
feature request Request for Workers team to add a feature jsg local dev

Comments

@dario-piotrowicz
Copy link
Member

dario-piotrowicz commented May 10, 2024

It would be great if the requests that the module fallback server receive could provide the original/raw/unaltered specifier exactly as it was defined in the referrer code.


To give more context we've been experimenting with using the module fallback service alongside the Vite environment api in order to lazily import cjs modules (which we could otherwise not import since vite does not perform any transformation on such code).

Meaning that, in workerd (specifically in the ModuleEvaluator's runExternalModule method), we receive a path such as the following from Vite:

/Users/me/project/node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react-jsx-dev-runtime.development.js

that we need to import and run.

The above works fine initially, but can easily break when the module in question includes require/import calls, such for example as:

var React = require('react');

and

var util = require('./utils.js');

The problem being that the above require calls produce respectively the following specifiers and referrers:

 specifier: /Users/me/project/node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react
 referrer: /Users/me/project/node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react-jsx-dev-runtime.development.js

and

 specifier: /Users/me/project/node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/utils
 referrer: /Users/me/project/node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react-jsx-dev-runtime.development.js

and from these we are unable to distinguish/differentiate the fact that react is referring to a dependency package and utils a local file (which for obvious reasons is fundamental for us).

So it would be necessary for us to also have the actual import specifier being used (i.e. "react" and "./utils.js" respectively)
(this would not introduce any breaking change if such specifier would be added to the request's search params or headers potentially as a raw-specifier/original-specifier or something along those lines).

Note

We could check the file extension to discern which is which but that is quite brittle as libraries can also end with .js etc...


As an example of the specifiers we have in mind, those should follow this comment: #1423 (comment)
(but again probably with a different search param name instead of specifier for backward compatibility)

@jasnell jasnell self-assigned this May 10, 2024
@IgorMinar
Copy link
Contributor

@jasnell I think the way the specifier is provided to the fallback service in it's semi-resolved way is a bug in workerd, a bug that vitest integration works around by post-processing the specifier: https://github.com/cloudflare/workers-sdk/blob/89b6d7f3832b350b470a981eb3b4388517612363/packages/vitest-pool-workers/src/pool/module-fallback.ts#L207-L231

I don't think that workerd can at all resolve the specifier provided by the user code's import in the cases where we hit the fallback service. In these cases the specifier needs to be resolved by an external service which needs to know what the original specifier, the referrer, and module system type used for the import. These three pieces of info can then be used by the fallback service to do module resolution and if a source for the given module is found return the source code.

Is it intentional that workerd currently does any post processing on the specifier and whether this is intentional or just accidental?

I do think that we want to change this but we'll need to do it in a backwards compatible way so that we don't break the current vitest integration and that's why dario is proposing that the fallback service returns the information using a new key, but other designs are also possible as long as we get the unprocessed specifier and we don't break vitest.

This issue is currently blocking the vite work, which can't proceed until this issue is resolved. Can you please give us an estimate for the fix so that we can plan our next steps? Thanks a bunch!

@jasnell
Copy link
Member

jasnell commented May 13, 2024

As discussed in chat, best estimate I can give right now is that I'm hoping to have time to look at the issue later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature jsg local dev
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants