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

Worker bundles #431

Closed
Andarist opened this issue Nov 19, 2021 · 6 comments · Fixed by #508
Closed

Worker bundles #431

Andarist opened this issue Nov 19, 2021 · 6 comments · Fixed by #508

Comments

@Andarist
Copy link
Member

To fix compatibility with workers, for example Cloudflare Workers, preconstruct needs to output bundles for workers target. This would fix emotion-js/emotion#2554 (comment)

There is no special top-level package.json for workers though - the only thing that is close to being treated as a "standard" is package.json#exports.worker. Webpack supports such a thing, see here and, in general, different tools can be configured to "consume" custom conditions from package.json#exports - for instance, in Rollup one could define those with an option for @rollup/plugin-node-resolve, like this: exportConditions: ['worker']. Some more context can be found here

We know that exports is quite dangerous, so this would have to be added very carefully. I'm not proposing to add any form of esm/cjs compat through exports. It would just be great if we could figure out a combination of conditions used there that wouldn't change anything for existing users but which would enable new use cases.

I think it would be great if preconstruct could support some special replacement strings, instead of process.env.NODE_ENV or typeof document (well, since I'm talking about a minor version - a new thing would have to be supported in addition to the old things). Stuff like import.meta.preconstruct.target could be supported.

On top of that it feels like, due to the static nature of ESM imports, it might not be enough to rely on "runtime" checks for all use cases. I propose to support additional suffixes on filenames that would act as aliases - for example index.worker.js could be picked with a priority, over index.js, when bundling for a worker target. This could be achieved with resolveId hook in Rollup

@emmatown
Copy link
Member

emmatown commented Dec 3, 2021

An idea WRT to the extension stuff: Instead of having stuff like .worker.js, we could respect using conditional exports in the imports field, this has the really nice advantage that it would just work with preconstruct dev.

@Andarist
Copy link
Member Author

Andarist commented Dec 3, 2021

Wouldnt it be too free-form though? It would also enforce people to use those # paths and this isnt yet working out-of-the-box with TS (although it will be in the future)

@nicksrandall
Copy link
Contributor

I opened (and linked) an issue on the Cloudflare wrangler2 cli repo to open discussion on how library authors should denote that a bundle is "workers" compatible.

@emmatown
Copy link
Member

emmatown commented Dec 9, 2021

Wouldnt it be too free-form though? It would also enforce people to use those # paths and this isnt yet working out-of-the-box with TS (although it will be in the future)

Maybe? I think the preconstruct dev working might be worth it. I think waiting for a TS version that supports the imports field would be fine though I suppose that could be problematic if the emitted declarations have the # imports and you want to support older TS versions.

Re the general thing though, I don't really think we need to solve this in Preconstruct super urgently. Given the browser bundles for Emotion are just more optimised, they don't use Node specific things, we could just add an exports field with a worker condition without changing anything in Preconstruct?

@nicksrandall re your PR, I imagine we'd want to use conditional exports, not a new top-level package.json field since I'm not aware of any bundlers that consume a top-level package.json field named worker but I believe webpack already supports a worker condition in conditional exports. So solving this would probably be after #432

@nicksrandall
Copy link
Contributor

@mitchellhamilton You're totally right that the "worker" top level keyword makes little sense since bundlers don't support that value. I've extended the PR to include support for package exports with custom conditionals (including "browser" and "worker").

@Andarist
Copy link
Member Author

An idea WRT to the extension stuff: Instead of having stuff like .worker.js, we could respect using conditional exports in the imports field, this has the really nice advantage that it would just work with preconstruct dev.

I think that I like the .worker.js convention better - but the package.json#imports has to be supported anyway (and adding the support for it is just a matter of configuring exportConditions appropriately for @rollup/plugin-node-resolve), so we can delay this conversation.

Re the general thing though, I don't really think we need to solve this in Preconstruct super urgently. Given the browser bundles for Emotion are just more optimised, they don't use Node specific things, we could just add an exports field with a worker condition without changing anything in Preconstruct?

Right, we don't need to fix this right away in Preconstruct but as show-cased, by the example here, this gets unwieldy pretty fast - so I would really like to avoid this being a manual addition to Emotion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants