In the module resolution playground app the remix route does not work in preview

[dario-piotrowicz]

If you run the module resolution playground app in dev mode everything seems to be working fine

But if you run it in preview mode and fetch from /third-party/remix you get the following error:

We should properly investigate why this happens and see if/how we can address this.

Ideally the app should prevent the exact same behavior in dev and preview.

[dario-piotrowicz]

Ok, I've investigated this and unfortunately this is a bit tricky...

TLDR: preview is correct in throwing, dev is correct in not throwing and I am not sure if we can align the two 😓

The issue is caused by this json call:

vite-plugin-cloudflare/playground/module-resolution/src/third-party/remix.ts

Line 4 in c5667eb

'(remix) typeof cloudflare json({})': typeof json({}),

The problem here is that this json call is an I/O operation being performed at the top level of the module, which is something that can't be done in workerd (internal discussion), so the error is actually the "correct"/expected behavior, and this is what happens in production builds/preview.

In dev, we never encounter this module as an actual ES module, since, based on the environment API, run it through runInlinedModule:

vite-plugin-cloudflare/packages/vite-plugin-cloudflare/src/runner/module-runner.ts

Line 52 in c5667eb

async runInlinedModule(context, transformed, module) {

(

)

And in this way the issue of this operation being at the top level of a dynamically imported module disappears, so here it is somewhat correct that the error is not thrown...

So unfortunately I think there is very little we can do about this (besides obviously removing the json call from the app...), which is pretty bad since the whole point of using the environment API is to align the dev behavior as close as possible to production :/

I will check with the runtime team to see if there is any way for us to trigger this error inside runInlinedModule somehow...

[jamesopstad]

Nice work getting to the bottom of this! On the plus this shouldn't happen for users with this json function as it will always be used inside a fetch handler. Agree that there are likely to be scenarios where users could run into this scenario though and that it would be good to be able to match the production behaviour in dev.

[jamesopstad]

It just occurred to me that if the dev behaviour was altered here to match production then this would create other problems for us. Currently, despite #64, we are able to use the HMR WebSocket at the top level of a module because this restriction is not enforced (see here for example - https://github.com/flarelabs-net/vite-plugin-cloudflare/blob/main/playground/hot-channel/src/index.ts). If this was changed then we wouldn't be able to use HMR at all so I think the current tradeoff is preferable.

[dario-piotrowicz]

(see here for example - https://github.com/flarelabs-net/vite-plugin-cloudflare/blob/main/playground/hot-channel/src/index.ts)

What code here would have issues with the restriction?

---

would create other problems for us

Yes that's a worry I have too, so as I mentioned in my workerd issue I think we should have a way to enable (if possible) this isolation and not to always enforce it, so that we can use potentially apply it only when appropriate

[jamesopstad]

(see here for example - https://github.com/flarelabs-net/vite-plugin-cloudflare/blob/main/playground/hot-channel/src/index.ts)

What code here would have issues with the restriction?

Wouldn't top-level code that uses a WebSocket (i.e. import.meta.hot) no longer be permitted?

[dario-piotrowicz]

(see here for example - https://github.com/flarelabs-net/vite-plugin-cloudflare/blob/main/playground/hot-channel/src/index.ts)

What code here would have issues with the restriction?

Wouldn't top-level code that uses a WebSocket (i.e. import.meta.hot) no longer be permitted?

I think that import.meta.hot.on would work just fine (but I might be wrong)

but this is a good point, if the code was isolated I wonder if the module would even get access to import.meta 😕

[dario-piotrowicz]

yeah I think that import.meta wouldn't even be accessible in the potentially restricted eval code 😕

So the addition of such a restriction might be a bad idea all around 😕

[jasnell]

... I wonder if the module would even get access to import.meta

It would not. unsafeEval compiles and runs the code as an ordinary script, not as a module, so import.meta would not be available.

[jasnell]

And to be certain, it's not actually possible to monkey patch import.meta in this way:

console.log(globalThis.import)  // undefined
globalThis.import = { meta: { hot: true }, };
console.log(globalThis.import.meta === import.meta);  // false

console.log(import.meta); 
console.log(global.import.meta);

The import.meta is a special internal exotic object that is defined by the host runtime. You can modify it directly, e.g. import.meta.foo = 1 but it is not actually a global so modifying it via global.import.meta would never work.

Also, import.meta is unique to every module, so modifications to import.meta made in one module would never be visible in the import.meta in a different module.

[dario-piotrowicz]

@jasnell yes, sorry such a stupid mistake on my part 😓 🙇 🤦

The gist of this in any case is that vite injects this import.meta.hot value (source (I think!)), that (I'm pretty sure) is done outside of the code evaluation (that we perform with the unsafe eval).

So the question is wether the import.meta.hot injected by Vite will be accessible from the unsafe evaled code if we were to apply the I/O context restriction, which from what you said seems to be a clear no

[dario-piotrowicz]

indeed I can see that behavior in the playground:

[jasnell]

Yep. It's because, like I said, import.meta is a unique object for every module. Your entrypoint import.meta is not going to be the same object as the import.meta that is accessible within your async-module.js.

[dario-piotrowicz]

Ok, so, in our plugin we can't really cause modules not to have access to import.meta.hot, so I think what I proposed in cloudflare/workerd#3218 is pretty much out of the question 😓
(and we'll just have to live with the dev / preview inconsistency)

Thanks a bunch for checking this and sorry for the inconvenience @jasnell 🙏 🙇