jest failing on react-scripts - needs a CJS build

[rafamel]

Hi again @agilgur5; since the code is published without transpiling, the ES module syntax is causing jest to error out, as node_modules are not transpiled by default. For an straightforwards fix, I'd suggest using @pika/pack`. I can do a PR with the whole setup if that would help :)

[agilgur5]

Huh, yeah I left it untranspiled as in 2019 I didn't think it was strictly necessary. Was thinking I could add support if some users needed it, but didn't expect jest to be a problem / still not have support for ESM. I often use ava which supports it out-of-the-box. I thought CRAv2+ (not sure if that aligns with react-scripts version) supported transpiling node_modules? Not sure if that's different from its jest support.

There's some solutions for ESM listed in https://github.com/facebook/jest/issues/4842 . If mst-persist is going to support ES5, I'd rather use tsdx to implement TS support simultaneously, esp. since MobX users overlap heavily with TS users

[rafamel]

Supporting TS sounds great. I was going to PR the definitions myself but if that's on the roadmap then I'm guessing there's no need.

Regarding CRA, create-react-app is just the cli that populates the project with react-scripts, so react-scripts is where the magic lives. Here's the jest config, which excludes node_modules transpilation (by default).

Regarding ava vs. jest & ESM, as tests run in node (most people are on v8 or v10 LTS), ESM need to be transpiled no matter what. Though it is also a common practice to exclude node_modules as transpilation time can take ages if all libraries are transpiled, hence most libraries publish several versions (ESM source, web bundle, node).

In a nutshell, that's what pika does. Regarding TS, since babel@7, TS can be transpiled to JS with babel, so you can go either way, tsdx seems great if you feel like it fits you best, but you can also accomplish the same w/ non TS specific tooling, provided you create the definition files w/ tsc. That being said, pika does also support tsc based transpiling. Take a look here for an example Typescript setup that transpiles to ESM, web bundle, node, and provides type definitions.

[agilgur5]

Supporting TS sounds great. I was going to PR the definitions myself but if that's on the roadmap then I'm guessing there's no need.

You can, but the TypeScript team themselves has explicitly said to publish types to DefinitelyTyped (@types) if the source code is not TS in https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html (c.f. https://github.com/inikulin/parse5/issues/235#issuecomment-362423707 and https://github.com/inikulin/parse5/issues/235#issuecomment-362446431).

It's on the roadmap but I don't necessarily have a timeline for it. If this issue is high prio, then I'll do it soon. Surface area is pretty small but everything takes time.

Regarding CRA, create-react-app is just the cli that populates the project with react-scripts, so react-scripts is where the magic lives. Here's the jest config, which excludes node_modules transpilation (by default).

Ok, gotcha. I don't use CRA and have my own webpack, etc configs. I thought react-scripts was specifically the ejected version. Version alignment is still relevant, but I think they're 1-to-1 in that case? In any case, I was referring to support for "packages written for latest Node versions" released in CRAv2. I guess that doesn't include testing support (per the config you linked) for some reason ¯\_(ツ)_/¯

Regarding ava vs. jest & ESM, as tests run in node (most people are on v8 or v10 LTS), ESM need to be transpiled no matter what. Though it is also a common practice to exclude node_modules as transpilation time can take ages if all libraries are transpiled, hence most libraries publish several versions (ESM source, web bundle, node).

AVA supports ESM using esm as per the same Jest issue. Node's rollout of ESM support has raised lots of issues with the community in general, and is partially why esm exists. And CRAv2's support for transpiling packages came out of discussions surrounding having compile steps and multiple builds (c.f. the link in the CRAv2 release). Minification used to be something library developers always did on builds, and now minification of all packages is standard in bundlers. I'm not saying build/test perf and CJS users aren't important, but there's some relevant context you seem to be missing -- there isn't really a standard for this.

In a nutshell, that's what pika does. Regarding TS, since babel@7, TS can be transpiled to JS with babel, so you can go either way, tsdx seems great if you feel like it fits you best, but you can also accomplish the same w/ non TS specific tooling, provided you create the definition files w/ tsc. That being said, pika does also support tsc based transpiling. Take a look here for an example Typescript setup that transpiles to ESM, web bundle, node, and provides type definitions.

Yes, I know babel supports TS now, but as I'm sure you know, it just strips types and doesn't do any type-checking. I'm not sold on pika for a variety of reasons, and if I'm going to be using a zero-config tool instead of my own configs, I'd rather use tsdx.

[agilgur5]

Ok, CJS and TS support are both in #8 . Still need to do some testing of both the types and behavior.

If you've got any time to spare to test within your project @rafamel it's on the ts branch. I could release a pre-release tag or possibly un-ignore the dist code to make testing easier.

[rafamel]

Hey @agilgur5 , sorry for the delay on responses, I've got the plate quite full these days. I wasn't aware of ESM ava support via the esm package; makes total sense. I agree in that there isn't really a standard on this, particularly w/ node's ESM rollout being so questionable, as you point out, though I do tend to include several builds in packages as i'd say issues like this are reasonably common. That said, I do agree w/ you in most of the points you make.

I'll take a look at the branch in a minute.

[agilgur5]

sorry for the delay on responses, I've got the plate quite full these days

no worries, I happened to have some time so decided to work on this. will probably go on "OSS hiatus" for a bit as spent a lot of time on OSS recently 😅 I thought breaking react-scripts/CRA testing was relatively high prio. Annnd now #9 causes some testing gotchas too welp 😖

I wasn't aware of ESM ava support via the esm package; makes total sense

Per the Jest issue I linked, Jest can support esm with its loaders as well. But apparently react-scripts/CRA doesn't support it out-of-the-box even in v2, which is kind of surprising -- perhaps worthwhile to file an issue there too.

i'd say issues like this are reasonably common

Sure and that's one solution. The whole discussion in the community has been whether or not that's a library author's responsibility -- and the answer has been no, and one can't really force authors into doing it, so the tooling has to support it. I've seem multiple ES6+ only packages nowadays. The lack of standard or best practice doesn't really help this scenario, like MST's package.json has a main, umd:main, module, browser, unpkg, jsnext:main, and react-native fields. Perhaps that's a bit unnecessary or perhaps this library should too 🤷‍♂ For now, as no one's asked for the others, I'll leave them out.

[agilgur5]

CJS builds are released in v0.1.0

[rafamel]

Just letting you know this is still a issue on CRA when using the default export instead of the named export (import persist from 'mst-persist') as tsdx doesn't mark the cjs build as an __esModule. While it works fine, in tests, as it's not an __esModule the default export is an object with default and persist keys, just as if imported with require. Just mentioning it as you might want to not export a default or drop tsdx in favor of a babel based cjs transpile for interop.

[agilgur5]

arggghhh, that's really annoying -- that means v0.1.0 has an unintentional breaking change... Thanks for reporting this issue!

I literally just saw this behavior in https://github.com/palmerhq/tsdx/issues/165, but didn't realize it affected mst-persist too as I thought the flag in tsconfig was enough and didn't know the output needed an __esModule flag when I checked it (now I do 😅). This bug seems to be caused right here, the comment of which makes it sound like an intentional decision, but it breaks compatibility pretty hard.

Should open this as a separate issue as CJS is now supported, but default exports in CJS isn't

At least a few of tsdx's assumptions don't hold up in test as #9 also breaks when testing with CJS (https://github.com/palmerhq/tsdx/issues/167) 😩