-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore(CRWA): convert to ESM #8159
Conversation
"build:watch": "nodemon --watch src --ignore dist,template --exec \"yarn build\"", | ||
"prepublishOnly": "NODE_ENV=production yarn build" | ||
"test": "yarn node --test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esbuild sets NODE_ENV
to production if minify is true, and here it is. See https://esbuild.github.io/api/#platform. (But more generally, I'm sure NODE_ENV
really does anything here.)
6079e3b
to
9f61b19
Compare
@@ -152,7 +161,7 @@ async function executeCompatibilityCheck(templateDir, yarnInstall) { | |||
*/ | |||
function checkNodeAndYarnVersion(templateDir) { | |||
return new Promise((resolve) => { | |||
const { engines } = require(path.join(templateDir, 'package.json')) | |||
const { engines } = fs.readJSONSync(path.join(templateDir, 'package.json')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use require
now, we'd have to create it using createRequire
.
9f61b19
to
1ff7499
Compare
// If you add, move, or remove a file from the create-redwood-app template, | ||
// you'll have to update this test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought snapshots were added to Node's test runner (or assert module), but I couldn't find them, so maybe I misread or they've been pulled out since
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like they were added here, but I can't find any docs on them: nodejs/node#44095
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment here hints that it was removed, but still can't find the commit: nodejs/node#47392 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found nodejs/node#46112 with it's linked comment
16 replays were recorded for 1ff7499. 16 PassedrequireAuth graphql checks
|
1ff7499
to
ac3849d
Compare
ac3849d
to
2cdba03
Compare
@@ -14,8 +15,7 @@ | |||
], | |||
"scripts": { | |||
"build": "yarn node ./build.mjs", | |||
"build:watch": "nodemon --watch src --ignore dist,template --exec \"yarn build\"", | |||
"prepublishOnly": "NODE_ENV=production yarn build" | |||
"test": "yarn node ./tests/template.test.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried yarn node --test
at first, but when we run yarn test
from the root, this command fails because it's fed bad options (jest related ones, like colors and maxWorkers). Something to figure out, but for now this works
I changed the esbuild |
@@ -14,8 +15,7 @@ | |||
], | |||
"scripts": { | |||
"build": "yarn node ./build.mjs", | |||
"build:watch": "nodemon --watch src --ignore dist,template --exec \"yarn build\"", | |||
"prepublishOnly": "NODE_ENV=production yarn build" | |||
"test": "yarn node --test-reporter spec ./tests/template.test.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set --test-reporter spec
because I was getting the tap reporter on my vscode terminal. I don't really think it'll make that much difference to switch it as we're not passing these test logs into anything programmatic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can tidy up things like the yarn test
command later as things evolve and become clearer. I don't think it holds this back.
import system from 'systeminformation' | ||
|
||
import { name as packageName, version as packageVersion } from '../package' | ||
// JSON modules aren't stable yet, but if they were we could use them instead. | ||
// See https://nodejs.org/dist/latest-v18.x/docs/api/esm.html#json-modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was interesting to me, so I did some digging.
It's still experimental even in Node 20: https://nodejs.org/docs/v20.0.0/api/esm.html#json-modules
It looks like it'll change syntax slightly before it fully ships (assert
will change to with
)
https://github.com/tc39/proposal-import-attributes
Scrolling to the very bottom there's an interesting "History" section. That section also mentions that compatibility with assert
will probably remain even after the official syntax switches to with
TIL 🙂
This was mostly an experiment; we learned a lot. This will still probably happen one way or another eventually (along with the rest of the packages) but this doesn't need to stay open at the moment. |
Edit: I've since added a
.babelrc.js
file for testing in a separate PR so the reasons for this PR detailed above aren't as valid. This package being a suitable build-tool playground is still pretty valid though.I want to add more unit tests to CRWA, but since I've removed Babel from the equation, I've also removed Jest. I could add back the
.babelrc.js
file, but CRWA is a really nice package to try things out in since its 1) a binary like the CLI, not a library (so it's never imported from) and 2) uses very few other@redwoodjs/*
as dependencies.So I want to try out writing tests using the new node test runner, and to do that in a nice way, this package needs to be ESM so that I can just import the functions straight from src.
I manually published the changes in this PR under a different package name (and then deleted that pacakge) just to try it out, and it worked using both yarn 1 and yarn 3. After merge we can confirm again via canary, then I could get started on more unit tests.