wooorm / direction

Detect directionality: left-to-right, right-to-left, or neutral
https://wooorm.com/direction/
MIT License
42 stars 8 forks source link

ESM + Jest #20

Closed milesj closed 3 years ago

milesj commented 3 years ago

Because this package is ESM only now and doesn't pre-compile, it complete breaks all unit testing since most (if not all) of them are unable to import ESM files.

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/en/ecmascript-modules for how to enable it.
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/milesj/Projects/aesthetic-react/node_modules/direction/index.js:16
    export function direction(value) {
    ^^^^^^

    SyntaxError: Unexpected token 'export'

      24 |
      25 | var React__default = /*#__PURE__*/_interopDefaultLegacy(React);
    > 26 |
         | ^
      27 | var getDirection__default = /*#__PURE__*/_interopDefaultLegacy(getDirection);
      28 |
      29 | var hoistNonReactStatics__default = /*#__PURE__*/_interopDefaultLegacy(hoistNonReactStatics);

      at Runtime.createScriptFromCode (../../node_modules/jest-runtime/build/index.js:1350:14)
      at Object.<anonymous> (../core-react/lib/index.js:26:20)
wooorm commented 3 years ago

ref: https://github.com/wooorm/markdown-table/issues/22.

I’ve slowly started pushing ESM from the low-level tools outwards.

The readme + these Jest links show some solutions.

Jest is having a lot of troubles with it though. Here’s one solution with TS and ESM: https://github.com/kentcdodds/mdx-bundler/blob/ee6c8d9981df93aa519f662617278e1ec3f9b2da/jest.config.js

wooorm commented 3 years ago

Closing as it works as expected, there’s ways around it, and you don’t have to change just yet if you don’t feel like it!

milesj commented 3 years ago

I disagree that it's working as intended. I'll just stick with v1 indefinitely.

wooorm commented 3 years ago

I'll just stick with v1 indefinitely.

What’s your reasoning for never wanting to use ESM?

milesj commented 3 years ago

This is a library that can be used in both web and node contexts, and should work in both of them without issue. As of right now, this is not the case. I'd also say that ESM only isn't offering any benefit over the v1 code.

IMO, it's also not a good pattern to expect all downstream consumers to reconfigure their tooling for a single library to be able to run.

wooorm commented 3 years ago

This is a library that can be used in both web and node contexts, and should work in both of them without issue.

ESM can be used in both. CJS can’t. This is specifically true for this project. For projects w/ dependencies, import maps (native, proposal) or a build step would still be needed. However, that’s already the case for CJS. ESM brings us closer to that interop you’re talking about.

As of right now, this is not the case

Incorrect. This project can be loaded in browsers. It couldn’t before.

I'd also say that ESM only isn't offering any benefit over the v1 code.

Not much, no, for this project. But the ecosystem moving towards a single import/export syntax is a good thing. I’m pretty happy with CJS, and would’ve been fine with not changing anything and instead browsers adopting it. But, ESM happened and it is better. It is the path forward.

IMO, it's also not a good pattern to expect all downstream consumers to reconfigure their tooling for a single library to be able to run.

I’m not suggesting to change for a single project. The shift is happening. Folks are changing to ESM. Even if that didn’t happen for existing tools, I think it’s allowed for developers of new projects to choose ESM. Users of dependencies will have to deal with it some time. And existing tools will change. Soon. Did you see https://twitter.com/sindresorhus/status/1349294527350149121?

milesj commented 3 years ago

@wooorm Yeah I'm well aware of the current state of modules in the frontend ecosystem. But that's also the problem, we're not 100% on ESM yet, so supporting both/all variants is good practice until we hit that threshold. Just my 2 cents.

I think once all popular tooling supports ESM natively, this will be a non-issue.

wooorm commented 3 years ago

I think once all popular tooling supports ESM natively, this will be a non-issue.

What’s the threshold here? When is it OK?

A lot of the tooling does already support it. Babel, Rollup, Vite, esbuild, tape, Node, ESLint, TS. I’ve had a great experience switching. April 30 is an important date, when Node 10 is EOL. Electron and Next are gearing up for that date too.

I’m seeing a couple of projects struggling: Jest and Webpack. IMO it’s really on them, they’ve supported faux-ESM for years and never bothering to make actual ESM work. It’s called ES2015 modules. These projects have had the time. I think a nudge might help push these projects to support ESM.

But that's also the problem, we're not 100% on ESM yet, so supporting both/all variants is good practice until we hit that threshold

I’ve had a terrible experience with dual publishing. Webpack 4 doesn’t handle it well. And there’s https://nodejs.org/api/packages.html#packages_dual_commonjs_es_module_packages.

All these projects (that I maintain, such as direction) used to have browser builds, Bower and Component.js manifests... It’s a lot of work.

For “until we hit that threshold”, agreed, but my perspective is that we reach that in 31 days

milesj commented 3 years ago

What’s the threshold here? When is it OK?

When they all support it. 🤷

This is all wishful thinking but not realistic. Don't mean to cause conflict, just the current state of things is non-ideal.

I'll just wait till everything catches up.

wooorm commented 3 years ago

just the current state of things is non-ideal.

Yep!

I'll just wait till everything catches up.

I think that’s totally valid!