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

Enable both CJS and ESM builds #18

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Enable both CJS and ESM builds #18

merged 5 commits into from
Oct 31, 2023

Conversation

mansona
Copy link
Member

@mansona mansona commented Sep 1, 2023

While working on #10 I was testing out setting up scenario-tester in a non-built package (ember-cli-fastboot) and I wasn't able to get scenario-tester list to work because the code always expected commonJS.

here is a PR to ember-cli-fastboot that implements scenario-tester using this PR 👍 ember-fastboot/ember-cli-fastboot#919

I had another attempt at this PR in #11 but that wasn't very extensive. This one tests that you can easily define your scenarios in ESM and also TS files 👍

I tried to implement the multi-build as described in https://thesametech.com/how-to-build-typescript-project/ but it turns out that this is impossible because of the way that scenario tester works. We rely on module state to "collect" scarios when we're running scenario-tester list but if you define your scenarios in CJS then the ESM in the binary won't have the same module state 😞 I guess we could augment the CJS or the ESM output and make sure they use the same module state but that seems a bit extreme for me.

It turns out all i needed to do to get ESM and CJS working nicely was to use global scope for collecting the scenarios 🎉 I think this is safe to release as a minor

@mansona mansona marked this pull request as draft September 1, 2023 14:02
@mansona mansona marked this pull request as ready for review September 1, 2023 14:30
@mansona mansona changed the title WIP support ESM Move to ESM Sep 1, 2023
@mansona mansona requested a review from a team September 1, 2023 14:32
@ef4
Copy link
Collaborator

ef4 commented Sep 1, 2023

We rely on module state to "collect" scarios when we're running scenario-tester list but if you define your scenarios in CJS then the ESM in the binary won't have the same module state

We could decide to make the module state into global state if we want to solve that problem. It really is global state, as you discovered.

But it may also be ok to be ESM-only. Since scenario-tester is generally running as a command and not used as a library within other projects, I think the cost of switching isn't high. People would be forced to port their tests to native-ESM, but it shouldn't impose any restrictions on their actual library or application code.

You'll need to check on this though. I would suggest publishing an alpha of scenario-tester and trying to fully integrate it with embroider to see if it goes smoothly.

import { resolve } from 'path';
import { format } from 'util';

import { createRequire } from 'node:module';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to find a lint for this, but I like having all the node builtins prefixed with node: -- good for differentiating between builtin and installed deps

@@ -1,24 +1,24 @@
import { Project } from 'fixturify-project';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a CJS TS? (I noticed there was an mjs -- porque no mts?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the mjs is just for a test to make sure that it can explicitly be imported correctly from a mjs file. I don't have any opinion on ts or mts really 🤷 other than to say that this PR adds type: module to the package.json so it's not required (in the same way that you only need mjs when you're not using type: module in package.json)

@mansona
Copy link
Member Author

mansona commented Sep 2, 2023

I would suggest publishing an alpha of scenario-tester and trying to fully integrate it with embroider to see if it goes smoothly.

I guess we don't need an alpha if we just depend on a git dependency 🤔 that's what I'm doing in ember-fastboot/ember-cli-fastboot#919 and it's working fine 👍

@mansona mansona self-assigned this Oct 2, 2023
@mansona mansona marked this pull request as draft October 27, 2023 12:26
@mansona mansona marked this pull request as ready for review October 27, 2023 14:01
@mansona
Copy link
Member Author

mansona commented Oct 27, 2023

i figured out how to make this work with both ESM and CJS support so we should be able to merge this soon 🎉

@mansona mansona changed the title Move to ESM Enable both CJS and ESM builds Oct 27, 2023
@mansona mansona force-pushed the mjs-support branch 4 times, most recently from eeafd12 to 7465483 Compare October 27, 2023 15:45
@mansona mansona force-pushed the mjs-support branch 3 times, most recently from 5b1ec6c to f1771e9 Compare October 27, 2023 15:56
@ef4 ef4 merged commit bf720b5 into main Oct 31, 2023
9 checks passed
@mansona mansona added the enhancement New feature or request label Oct 31, 2023
@mansona mansona deleted the mjs-support branch January 8, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants