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

chore(CRWA): convert to ESM #8159

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/create-redwood-app/build.mjs
Expand Up @@ -8,13 +8,15 @@ import * as esbuild from 'esbuild'
// instead of using the catch-all `packages: 'external'` option.
const result = await esbuild.build({
entryPoints: ['src/create-redwood-app.js'],
format: 'esm',
bundle: true,
platform: 'node',
target: ['node18'],
// See https://esbuild.github.io/getting-started/#bundling-for-node.
packages: 'external',
outfile: 'dist/create-redwood-app.js',
minify: true,
logLevel: 'info',

// For visualizing the bundle.
// See https://esbuild.github.io/api/#metafile and https://esbuild.github.io/analyze/.
Expand Down
4 changes: 2 additions & 2 deletions packages/create-redwood-app/package.json
@@ -1,6 +1,7 @@
{
"name": "create-redwood-app",
"version": "5.0.0",
"type": "module",
"repository": {
"type": "git",
"url": "https://github.com/redwoodjs/redwood.git",
Expand All @@ -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"
Copy link
Collaborator

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?

https://nodejs.org/api/test.html#test-reporters

},
"dependencies": {
"@opentelemetry/api": "1.4.1",
Expand Down
19 changes: 14 additions & 5 deletions packages/create-redwood-app/src/create-redwood-app.js
@@ -1,6 +1,7 @@
#!/usr/bin/env node

import path from 'path'
import path from 'node:path'
import { fileURLToPath } from 'node:url'

import { trace, SpanStatusCode } from '@opentelemetry/api'
import chalk from 'chalk'
Expand All @@ -14,13 +15,21 @@ import yargs from 'yargs/yargs'

import { RedwoodTUI, ReactiveTUIContent, RedwoodStyling } from '@redwoodjs/tui'

import { name, version } from '../package'

// In ESM, for relative paths, the extension is important.
// See https://nodejs.org/dist/latest-v18.x/docs/api/esm.html#mandatory-file-extensions.
import {
startTelemetry,
shutdownTelemetry,
recordErrorViaTelemetry,
} from './telemetry'
} from './telemetry.js'

// 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.
const __dirname = path.dirname(fileURLToPath(import.meta.url))

const { name, version } = fs.readJSONSync(
path.resolve(__dirname, '../package.json')
)

// Telemetry
const { telemetry } = Parser(hideBin(process.argv))
Expand Down Expand Up @@ -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'))
Copy link
Contributor Author

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.


checkNodeVersionCb(engines, (_error, result) => {
return resolve([result.isSatisfied, result.versions])
Expand Down
10 changes: 9 additions & 1 deletion packages/create-redwood-app/src/telemetry.js
@@ -1,3 +1,6 @@
import path from 'node:path'
import { fileURLToPath } from 'node:url'

import { diag, DiagConsoleLogger, DiagLogLevel } from '@opentelemetry/api'
import opentelemetry, { SpanStatusCode } from '@opentelemetry/api'
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http'
Expand All @@ -9,9 +12,14 @@ import {
import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'
import ci from 'ci-info'
import envinfo from 'envinfo'
import fs from 'fs-extra'
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.
Copy link
Member

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 🙂

const { name: packageName, version: packageVersion } = fs.readJSONSync(
path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../package.json')
)

/**
* @type NodeTracerProvider
Expand Down
106 changes: 106 additions & 0 deletions packages/create-redwood-app/tests/template.test.js
@@ -0,0 +1,106 @@
/* eslint-env node, es2022 */

import assert from 'node:assert'
import fs from 'node:fs'
import path from 'node:path'
import { test } from 'node:test'
import { fileURLToPath } from 'node:url'

const TEMPLATE_PATH = path.resolve(
path.dirname(fileURLToPath(import.meta.url)),
'../template'
)

const EXPECTED_FILES = [
'/.editorconfig',
'/.env',
'/.env.defaults',
'/.env.example',
'/.nvmrc',
'/.vscode/extensions.json',
'/.vscode/launch.json',
'/.vscode/settings.json',
'/.yarn/releases/yarn-3.5.0.cjs',
'/.yarnrc.yml',
'/README.md',
'/api/db/schema.prisma',
'/api/jest.config.js',
'/api/package.json',
'/api/server.config.js',
'/api/src/directives/requireAuth/requireAuth.test.ts',
'/api/src/directives/requireAuth/requireAuth.ts',
'/api/src/directives/skipAuth/skipAuth.test.ts',
'/api/src/directives/skipAuth/skipAuth.ts',
'/api/src/functions/graphql.ts',
'/api/src/graphql/.keep',
'/api/src/lib/auth.ts',
'/api/src/lib/db.ts',
'/api/src/lib/logger.ts',
'/api/src/services/.keep',
'/api/tsconfig.json',
'/gitignore.template',
'/graphql.config.js',
'/jest.config.js',
'/package.json',
'/prettier.config.js',
'/redwood.toml',
'/scripts/.keep',
'/scripts/seed.ts',
'/scripts/tsconfig.json',
'/web/jest.config.js',
'/web/package.json',
'/web/public/README.md',
'/web/public/favicon.png',
'/web/public/robots.txt',
'/web/src/App.tsx',
'/web/src/Routes.tsx',
'/web/src/components/.keep',
'/web/src/index.css',
'/web/src/index.html',
'/web/src/layouts/.keep',
'/web/src/pages/FatalErrorPage/FatalErrorPage.tsx',
'/web/src/pages/NotFoundPage/NotFoundPage.tsx',
'/web/tsconfig.json',
].sort()

// If you add, move, or remove a file from the create-redwood-app template,
// you'll have to update this test.
Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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)

Copy link
Collaborator

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

test("the file structure hasn't unintentionally changed", () => {
// Sort because files are not always reported in the same order as the list
assert.deepStrictEqual(walk(TEMPLATE_PATH).sort(), EXPECTED_FILES)
})

/**
* Get all the files in a directory.
*
* @param {string} dir
* @returns string[]
*/
function walk(dir) {
/** @type {string[]} */
let files = []

const fileList = fs.readdirSync(dir)

fileList.forEach((file) => {
file = path.join(dir, file)

const stat = fs.statSync(file)

if (!stat) {
throw new Error(`Failed to get stats for ${file}`)
}

if (stat.isDirectory()) {
// Recurse into directory
files = files.concat(walk(file))
} else {
// It's a file
files.push(file)
}
})

return files.map((file) =>
file.replace(TEMPLATE_PATH, '').split(path.sep).join(path.posix.sep)
)
}