Skip to content

Commit

Permalink
fix(esm): use CJS wrapper for ESM default interop (#10119)
Browse files Browse the repository at this point in the history
This PR builds on the work started in
#10083. It does two things:

- ~builds the `@redwoodjs/vite` package with esbuild instead of babel
(this is optional but just did it to debug, but I'd imagine we'd want to
start doing this anyway)~
  - CI wasn't happy with this change so removed for now
- introduces a CJS wrapper to handle ESM export default interoperation
in Node

To frame this change at a high level, one of the ways I'm approaching
ESM is backwards from Redwood apps. If Redwood apps can be ESM (via
"type": "module" in package.jsons, updates to tsconfigs, adding file
extensions to relative imports, etc), then we may have an easier time
rolling out packages that can't be shipped as dual ESM/CJS exports.
(Vite is probably one of them. But since Vite evaluates the config file
itself, it may be ok.)

One of the framework-level bugs that shows up when you make an app ESM
is this error:

```
failed to load config from ~/redwood/redwood-app-esm/web/vite.config.mts
file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918-
6f6a7f8efad05.mjs:7
  plugins: [redwood()]
            ^

TypeError: redwood is not a function
    at file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918-
6f6a7f8efad05.mjs:7:13
```

The culprit is this import in `web/vite.config.ts`:

```ts
import redwood from '@redwoodjs/vite'
```

The problem is confusing, but basically... when you 1) transpile an ES
module into CJS (which is what we're doing for most of our packages) and
then 2) import that CJS module from a bona fide ES module, the default
export doesn't behave how you'd expect. Evan Wallace has a great write
up of it
[here](https://esbuild.github.io/content-types/#default-interop). (For
Node's official docs, see
https://nodejs.org/docs/latest/api/esm.html#commonjs-namespaces.)

As a potential fix, @Tobbe pointed me to this Vite plugin:
https://github.com/cyco130/vite-plugin-cjs-interop. I tried adding it
but the issue is that this **is** the Vite config file. 😅

But I thought I may be able to add the plugin here when we call
`createServer`:

https://github.com/redwoodjs/redwood/blob/e9ecbb07da216210c59a1e499816f31c025fe81d/packages/vite/bins/rw-vite-dev.mjs#L28-L37

But it still wasn't working. I stepped through the source a bit and it
doesn't seem like there's room for configuring how Vite loads the config
file. The two main functions were these, `loadConfigFromBundledFile` and
`loadConfigFromFile`:
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L1186
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L962

`bundleConfigFile` has plugins configured, but they're hardcoded.

But luckily I don't think it's necessary... based on this esbuild
thread, it seems like we can get away with a small wrapper. See
evanw/esbuild#532. When we actually make this
an ES module (which will happen pretty soon I think), this will go away.
But for the time being, this is a CJS module.
  • Loading branch information
jtoar authored and ahaywood committed Mar 8, 2024
1 parent 1a8d5e3 commit a9de838
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 117 deletions.
108 changes: 14 additions & 94 deletions CHANGELOG.md
@@ -1,7 +1,13 @@
# CHANGELOG

## Unreleased

- fix(esm): use CJS wrapper for ESM default interop (#10119)

This PR builds on the work started in https://github.com/redwoodjs/redwood/pull/10083 around ESM. One of the caveats of that PR was that the default export from `@redwoodjs/vite` broke. The workaround was referencing the `default` property on the Redwood Vite plugin, like `redwood.default()`. This fixes the ES module default export interoperability so that no change is necessary in switching between module types.

- feature: Enable [CSS nesting](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_nesting/Using_CSS_nesting) syntax by default when using Tailwind:

```
.button {
@apply p-2 font-semibold bg-gray-500;
Expand All @@ -17,105 +23,19 @@
}
```

## Unreleased
- fix(api-server): Preserve original host header for proxied API requests
Some apps rely on reading the host header(eg multi-tenant apps served over multiple subdomains). This change forwards on the original host header on proxied Fastify requests, and the experimental SSR/RSC server

- fix(context): Re-export context from graphql-server (#10117)

This change re-exports the `context` and `setContext` properties in
`@redwoodjs/graphql-server` from the `@redwoodjs/context` package
where they are now (as of v7) located. This is done to retroactively
ease the v7 transition and provide a non-breaking rather than a breaking
change.

See [this forum post](https://community.redwoodjs.com/t/context-imported-from-graphql-server-broken-post-7-0-0/5833)
and the links within for more information on this change.

- fix(scenario): Make sure to clean up scenarios even if tests fail (#10112)
Fixes an issue where a unit test failure would cause the scenario cleanup to be skipped. Thanks @peraltafederico and @cjreimer for highlighting this!

- fix(serve): Allow periods in most paths (#10114)

Partial fix for route paths with periods in them.

It's only "partial" because it doesn't fix it for `yarn rw dev`, as that's a
Vite bug
([vitejs/vite#2415 (comment)](https://github.com/vitejs/vite/issues/2415#issuecomment-1720814355)).
And there's also an edge case for yarn rw serve where this doesn't fully
handle client-side routes that start with /assets/ and that also have a
last-segment that accepts a period, like /assets/client-route-image.jpg

Fixes #9969
## v7.1.0

- fix(deps): update prisma monorepo to v5.10.2 (#10088)
- See https://github.com/redwoodjs/redwood/releases/tag/v7.1.0

This release updates Prisma to v5.10.2. Here are quick links to all the release notes since the last version (v5.9.1):
## v7.0.7

- https://github.com/prisma/prisma/releases/tag/5.10.0
- https://github.com/prisma/prisma/releases/tag/5.10.1
- https://github.com/prisma/prisma/releases/tag/5.10.2

- fix(esm): fix initial ESM blockers for Redwood apps (#10083) by @jtoar and @Josh-Walker-GM

This PR makes some initial fixes that were required for making a Redwood app ESM. Redwood apps aren't ready to transition to ESM yet, but we're working towards it and these changes were backwards-compatible.
If you're interested in trying out ESM, there will be an experimental setup command in the future. For now you'd have to make manual changes to your project:

- dist imports, like `import ... from '@redwoodjs/api/logger'` need to be changed to `import ... from '@redwoodjs/api/logger/index.js'`
- The Redwood Vite plugin in `web/vite.config.ts` needs to be changed to `redwood.default` before being invoked

There are probably many others still depending on your project. Again, we don't recommend actually doing this yet, but are enumerating things just to be transparent about the changes in this release.

- fix(deploy): handle server file (#10061)

This fixes the CLI commands for Coherence and Flightcontrol. For Coherence, it fixes a bug introduced in the last patch where the logic for detecting the server file in the setup command (`yarn rw setup deploy coherence`) was flipped. For Flightcontrol, it updates the setup command (`yarn rw setup deploy flightcontrol`) so that it handles Corepack and updates the corresponding deploy command (`yarn rw deploy flightcontrol`) so that it detects the server file similar to the Coherence fix.

- chore(docs): Add link to SuperTokens auth (#10067)

Add a missing link to the SuperTokens auth page in the docs. @danbtl

- fix(coherence): update setup command to detect server file

The `yarn rw setup deploy coherence` command now detects if your project has the server file and configures the api prod command accordingly:

```yml
# coherence.yml

api:
# ...
prod:
command: ["yarn", "rw", "build", "api", "&&", "yarn", "node", "api/dist/server.js", "--apiRootPath=/api"]
```
- See https://github.com/redwoodjs/redwood/releases/tag/v7.0.7

- Update jsdoc for ScenarioData type (#29166)
## v7.0.6

Fix formatting of JSDocs in `scenario.ts`
- See https://github.com/redwoodjs/redwood/releases/tag/v7.0.6

- fix(render): reduce memory and handle server file

This PR improves Render deploys by reducing memory consumption and fixing it so that it uses the server file if it's present.

Render deploys seems to consistently run out of memory during the data migration step. This step is configurable and its doubtful that every deploy has data migrations to apply, but it's enabled by default so it runs every time. The main issue is that the data migrate functionality is a plugin so a yarn install kicks off in Render's deploy container which must be more memory-constrained than the build container. (Assuming there are two different containers, which seems to be the case.)

Instead of running data migrations, this PR issues a warning that if you want to run data migrations, you need to first add the `@redwoodjs/cli-data-migrate` package as a devDependency:

```
yarn add -D @redwoodjs/cli-data-migrate
```

That way a `yarn install` won't be necessary to run data migrations.

Although this PR fixes Render deploy so that it uses the server file if present, realtime features still don't seem to work. We're still investigating; in the meantime, consider using another provider like Coherence if you're just getting started and want to try out realtime features.

- Update MetaTags to be Metadata in Docs (#10053)

The tutorial still used the `MetaTags` component instead of the newer `Metadata` component that the generator templates use. This PR updates all instances of `MetaTags` with `Metadata`.

- fix(sentry): move templates to the command's directory

Fix for https://community.redwoodjs.com/t/redwood-v7-0-0-upgrade-guide/5713/25. The template files for the sentry setup command weren't moved out of experimental (follow up to https://github.com/redwoodjs/redwood/pull/9830).

## v7.0.0
## v7.0.5

- See https://github.com/redwoodjs/redwood/releases/tag/v7.0.0 for the release notes and https://community.redwoodjs.com/t/redwood-v7-0-0-upgrade-guide/5713 for the upgrade guide

Expand Down
3 changes: 3 additions & 0 deletions packages/vite/cjsWrapper.js
@@ -0,0 +1,3 @@
/* eslint-env node */

module.exports = require('./dist/index.js').default
47 changes: 24 additions & 23 deletions packages/vite/package.json
@@ -1,30 +1,9 @@
{
"name": "@redwoodjs/vite",
"version": "7.1.0",
"description": "Vite configuration package for Redwood",
"repository": {
"type": "git",
"url": "https://github.com/redwoodjs/redwood.git",
"directory": "packages/vite"
},
"license": "MIT",
"main": "dist/index.js",
"bin": {
"rw-vite-build": "./bins/rw-vite-build.mjs",
"rw-vite-dev": "./bins/rw-vite-dev.mjs",
"vite": "./bins/vite.mjs"
},
"files": [
"dist"
],
"scripts": {
"build": "yarn build:js && yarn build:types",
"build:js": "babel src -d dist --extensions \".js,.jsx,.ts,.tsx\"",
"build:pack": "yarn pack -o redwoodjs-vite.tgz",
"build:types": "tsc --build --verbose",
"test": "vitest run src",
"test:watch": "vitest watch src"
},
"dependencies": {
"@babel/runtime-corejs3": "7.24.0",
"@redwoodjs/babel-config": "7.1.0",
Expand All @@ -36,6 +15,7 @@
"vite": "4.5.2",
"yargs-parser": "21.1.1"
},
"description": "Vite configuration package for Redwood",
"devDependencies": {
"@babel/cli": "7.23.9",
"@types/react": "^18.2.55",
Expand All @@ -44,5 +24,26 @@
"typescript": "5.3.3",
"vitest": "1.2.2"
},
"gitHead": "3905ed045508b861b495f8d5630d76c7a157d8f1"
}
"files": [
"dist",
"cjsWrapper.js"
],
"gitHead": "3905ed045508b861b495f8d5630d76c7a157d8f1",
"license": "MIT",
"main": "cjsWrapper.js",
"name": "@redwoodjs/vite",
"repository": {
"directory": "packages/vite",
"type": "git",
"url": "https://github.com/redwoodjs/redwood.git"
},
"scripts": {
"build": "yarn build:js && yarn build:types",
"build:js": "babel src -d dist --extensions \".js,.jsx,.ts,.tsx\"",
"build:pack": "yarn pack -o redwoodjs-vite.tgz",
"build:types": "tsc --build --verbose",
"test": "vitest run src",
"test:watch": "vitest watch src"
},
"version": "7.1.0"
}

0 comments on commit a9de838

Please sign in to comment.