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

Dependencies need to be ESM #153

Closed
4 tasks done
benmccann opened this issue Jul 20, 2021 · 6 comments
Closed
4 tasks done

Dependencies need to be ESM #153

benmccann opened this issue Jul 20, 2021 · 6 comments
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on

Comments

@benmccann
Copy link

Initial checklist

Affected packages and versions

all

Link to runnable example

No response

Steps to reproduce

Provided in sveltejs/kit#1961

Expected behavior

All dependencies should be ESM since this package declares "type": "module". The package should work out-of-the-box with SvelteKit and Vite

This can be fixed by distributing an ESM version of is-buffer and extend or by removing those dependencies from this library since they have minimal usage. I've sent a PR for is-buffer feross/is-buffer#43 though I don't know yet whether it will be accepted

This could be added a sub-task to #121

Actual behavior

Vite encounters CJS dependency of this ESM module and fails with the message module is not defined as described in vitejs/vite-plugin-react#30

Runtime

Node v14, Other (please specify in steps to reproduce)

Package manager

npm v7

OS

Linux

Build and bundle tools

Vite

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 20, 2021
@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 21, 2021

Thanks for reaching out @benmccann!

It's also worth noting the Vite is planning to fix this so it would work by default vitejs/vite#3024 (comment) and there is a work around available today without needing to change packages vitejs/vite#3024 (comment)


This can be fixed by distributing an ESM version of is-buffer and extend

I'm happy to see improvements in unified's dependencies. 👍
Thanks for opening feross/is-buffer#43!
Would you be interested in opening a pull request to extend as well?


by removing those dependencies from this library since they have minimal usage

I'm happy to change unified's dependencies, if there are security, performance, or stability benefits. 👍

I'm cautious changing dependencies only for the sake of working around a short term bug in a particular tool chain.
As more packages adopt ESM, module format bugs in several tool chains have been uncovered (most outlined with workarounds in: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c)

We definitely want to offer a path for folks to upgrade across a variety of tool chains.
Baking workarounds into unified long term to address short term bugs in particular build tools, doesn't feel like an appropriate or sustainable approach.
For short term problems, where upstream fixes are not immediately possible, documentation/guides feel more appropriate.

Happy to discuss dependency changes or help with documentation!

@raulfdm
Copy link

raulfdm commented Jul 21, 2021

@ChristianMurphy , @benmccann ... I've opened this PR to help out: justmoon/node-extend#57

@wooorm
Copy link
Member

wooorm commented Jul 21, 2021

All dependencies should be ESM since this package declares "type": "module". The package should work out-of-the-box with SvelteKit and Vite

No, I disagree. type: module is a Node.js thing. Node allows CJS dependencies. If your bundler supports type: module, then it should also allow CJS dependencies.

Of course: hopefully in the future we can ditch CJS! But Vite is broken. Not unified.


As Christian mentioned, we can discuss how to teach folks how to use Vite with npm and unified, but I’m closing this because it’s not actionable.

@wooorm wooorm closed this as completed Jul 21, 2021
@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Jul 21, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Jul 21, 2021
@205g0
Copy link

205g0 commented Nov 11, 2021

For short term problems, where upstream fixes are not immediately possible, documentation/guides feel more appropriate.
...
we can discuss how to teach folks how to use Vite with npm and unified, but I’m closing this because it’s not actionable.

This would be helpful, I can't get unified to work in the SSR part of SvelteKit (Vite-based): I get __vite_ssr_import_2__.unified is not a function

When I have:

vite: {
  optimizeDeps: {
    include: ['unified'],
  },
},

I get module is not defined at ...is-buffer...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

5 participants