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

Discussion: enable mix of cjs and es test code. #155

Closed
3cp opened this issue Jun 16, 2022 · 5 comments · Fixed by #156
Closed

Discussion: enable mix of cjs and es test code. #155

3cp opened this issue Jun 16, 2022 · 5 comments · Fixed by #156

Comments

@3cp
Copy link
Contributor

3cp commented Jun 16, 2022

Following up #153, there is an easy way to enable mix of cjs and es test codes, report all of them as one.

Currently pta has to use --module-loader to separate cjs and es zora instance. The reason of that separation is that zora package shipped two dist files, one for cjs, one for es.

I tested following change, it all worked out:

  1. don't bundle zora for es dist.
  2. supply a hard-coded es dist file (may not need to be in dist folder) with following content:
export {
  Assert, createHarness, createJSONReporter, createTAPReporter,
  hold, only, report, skip, test
} from './index.cjs';

In this way, no matter how zora is used, the same zora instance in cjs file is used. Even when you mix test/a.spec.cjs and test/b.spec.mjs, it's still the same zora instance used across cjs and es tests, hence one united report will be generated.

The other benefit is it can remove the --module-loader option from pta, there is no function lost, just less friction for end users.

@3cp
Copy link
Contributor Author

3cp commented Jun 16, 2022

Oh, this is enough. And better for future improvements.

export * from './index.cjs';

Reducing zora to a commonjs only package also works. (removing "exports" in package.json).
But that's a breaking change (removed zora/cjs and zora/es).

@lorenzofox3
Copy link
Owner

lorenzofox3 commented Jun 23, 2022

This sounds interesting. I am a bit concerned that it implies committing to cjs where the global trend is on the contrary get rid of it. Have you tried outside of pta ? browser, simple node module, node regular cjs package etc.

Would you mind providing a PR so I get a better picture ?

@3cp
Copy link
Contributor Author

3cp commented Jun 23, 2022

Browser is different, nodejs module resolving is not in play.
All web app bundlers will try package.json fields (by default order) "module", "browser", "main" regardless what "type" says.

(Not exactly sure about latest development of bundlers on dealing with "exports" field.)

Note "module" and "browser" are not in nodejs/npm spec, they are purely invented by browserify and webpack I think.

I used zora in browser. Pta is irrelevant in that contex. The bundler would not bundle two zora (cjs and esm). Bundler resolves require('zora') and import ... from 'zora' to same file.

I tested pta and zora in many different setups, src in cjs or esm, test in cjs or esm, all combinations work well. I have converted quite a few projects from tape or ava to zora. Really enjoy zora.

All require() or import from or import() can be used to consume a cjs npm package. But only import from or import() for esm npm package. That's very annoying.

In nodejs, wrapping cjs into esm is easy. The other way is unfortunately prohibited (otherwise much better choice). I'd rather Nodejs folks sided with usability rather than perfect spec conformance.

I will raise a PR.

@3cp 3cp mentioned this issue Jun 23, 2022
@lorenzofox3
Copy link
Owner

Oh thanks, much appreciated. I'll get a look. From the browser, is it still possible to get zora working with no bundler (ie with a simple URL from a CDN for example) ?

@3cp
Copy link
Contributor Author

3cp commented Jun 24, 2022

Sure, you mean native es module. Then cjs is out of picture anyway.

This reminded me the wrapper may not work for browser, at least for those cdns serving plain npm files.

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

Successfully merging a pull request may close this issue.

2 participants