unifiedjs / unified

☔️ interface for parsing, inspecting, transforming, and serializing content through syntax trees
https://unifiedjs.com
MIT License
4.49k stars 110 forks source link

Dependencies need to be ESM #153

Closed benmccann closed 3 years ago

benmccann commented 3 years ago

Initial checklist

Affected packages and versions

all

Link to runnable example

No response

Steps to reproduce

Provided in https://github.com/sveltejs/kit/issues/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 https://github.com/feross/is-buffer/pull/43 though I don't know yet whether it will be accepted

This could be added a sub-task to https://github.com/unifiedjs/unified/issues/121

Actual behavior

Vite encounters CJS dependency of this ESM module and fails with the message module is not defined as described in https://github.com/vitejs/vite-plugin-react/issues/30

Runtime

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

Package manager

npm v7

OS

Linux

Build and bundle tools

Vite

ChristianMurphy commented 3 years ago

Thanks for reaching out @benmccann!

It's also worth noting the Vite is planning to fix this so it would work by default https://github.com/vitejs/vite/issues/3024#issue-860269204 and there is a work around available today without needing to change packages https://github.com/vitejs/vite/issues/3024#issuecomment-860018367


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

I'm happy to see improvements in unified's dependencies. :+1: 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. :+1:

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 commented 3 years ago

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

wooorm commented 3 years ago

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.

github-actions[bot] commented 3 years ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

github-actions[bot] commented 3 years ago

Hi team! Could you describe why this has been marked as external?

Thanks, — bb

205g0 commented 3 years ago

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...