fix(spectral-config): partially revert ESM #724
Conversation
This reverts commit b1a4ee5.
| "engines": { | ||
| "node": ">=16" | ||
| }, | ||
| "scripts": { | ||
| "lint": "eslint .", | ||
| "test": "vitest run" | ||
| "test": "NODE_OPTIONS='--no-deprecation' vitest run" |
There was a problem hiding this comment.
I really despise this, but without it the test output looks like this:
Unfortunately it's all or nothing here and there's no way to hide a specific warning (see nodejs/node#40940).
I opened up get-alex/alex#342 which should hopefully address this.
| /** | ||
| * Ensure that a given string has considerate and inclusive language. | ||
| * | ||
| * @see {@link https://alexjs.com/} | ||
| * @param {string} input | ||
| */ | ||
| export default function alex(input, options, context) { | ||
| module.exports = async function alex(input, options, context) { | ||
| const { text } = await import('alex'); |
There was a problem hiding this comment.
This is pretty much the only actual code change in this PR (everything else is pretty much a revert). Since the Spectral CLI (though claiming otherwise) requires us to use CommonJS and alex@11 is ESM, we need to dynamically import alex here.
There was a problem hiding this comment.
If you change this to alex/index.js does that fix that gnarly ESM error that shows up when running tests?
There was a problem hiding this comment.
wait what gnarly error are you seeing when running tests 🧐
There was a problem hiding this comment.
oh right 🤦🏽 let me see!
| @@ -1,5 +1,4 @@ | |||
| { | |||
| "extends": "@readme/eslint-config", | |||
| // required for ESM | |||
| "rules": { "import/extensions": ["error", "always"] } | |||
| "parserOptions": { "ecmaVersion": 2020 } | |||
There was a problem hiding this comment.
This is required for us to use the dynamic import in packages/spectral-config/src/functions/alex.js. I love JavaScript, perfect ecosystem, no notes, etc etc etc
| @@ -1,9 +1,10 @@ | |||
| import { makeCopy, severityCodes, testRule } from '@ibm-cloud/openapi-ruleset/test/utils/index.js'; | |||
| import { makeCopy, severityCodes, testRule } from '@ibm-cloud/openapi-ruleset/test/utils'; | |||
There was a problem hiding this comment.
we need to drop these /index.js chunks because the ESLint config changes required for the dynamic import. another grumbling about JS etc etc etc
🧰 Changes
Unfortunately the
spectral-configESM migration (see #718) doesn't actually play nicely with Spectral, despite their claim that they support ESM 🤬In this PR, I mostly converted this package back to CommonJS and instead dynamically imported
[email protected], which is ESM-only. That way we can take advantage of the latest version ofalexwhile (hopefully) playing nicely with Spectral. And thanks to #723,vitesthandles this mess flawlessly.I think this can be a non-breaking change (i.e.,
[email protected]) since the Node 16+ requirements are the same and there are no changes in the way that we load this into Spectral.🧬 QA & Testing
Honestly I tried
npm link-ing this package and... it still didn't work 🙃 the Spectral CLI is still saying we should use a dynamic import, despite that being exactly what we're doing 😵💫I have some skepticism about
npm-linkthough so... it might work if we publish? I'm not super optimistic though. Honestly the current state of the package is totally broken anyways, so it's not like we can break it any further ¯_(ツ)_/¯ if this fails then I'll do a total revert and go back toalex@9.