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

Module resolution, switched testing to vitest #1211

Closed
wants to merge 8 commits into from

Conversation

ciscoheat
Copy link

This modernizes the package in a few ways:

  • Adding ".js" extensions to import statements, for modern bundler compatibility.
  • Switched to vitest testing framework, to get rid of babel.

mcmire added a commit to MetaMask/superstruct that referenced this pull request Feb 9, 2024
Superstruct cannot be imported in a TypeScript project that uses a
`moduleResolution` of `node16` or `nodenext`. Any attempt to do so will result
in type errors such as the following:

```
node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 export * from './error';
                ~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './struct';
                ~~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:3:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

3 export * from './structs/coercions';
                ~~~~~~~~~~~~~~~~~~~~~

...
```

This is happening because although Superstruct is defined as an ESM library —
the `module` field in `package.json` is set to `true` — its published TypeScript
declaration files aren't ESM-compatible. This is due to the fact that imports
are missing extensions, which is a requirement for ESM.

At the same time, although Rollup is supposedly configured to emit both ESM- and
CJS-compatible files, TypeScript does not know how to use the CJS variants. The
`exports` field is the way to instruct TypeScript on how to do this, but
`package.json` is missing this field. This makes it impossible to use
Superstruct in a non-ESM library.

This commit fixes both of these issues by making the following changes:

- All imports in TypeScript files now use an extension of `.js`. This may seem
  like an odd choice, as there are no JavaScript files in the `src/` directory,
  but TypeScript doesn't resolve a module by trying to find a file with the
  given extension; it tries to find a declaration file that maps to the module
  path, based on a set of rules. Therefore, the file extension is really just a
  formality. In the end, Rollup is consolidating implementation files into one,
  so all of the imports will go away.
- Type declaration files have been split into ESM and CJS variants. Since Rollup
  generates ESM variants with an `.mjs` extension and CJS variants with a `.cjs`
  extension, imports in declaration files also use either `.mjs` or `.cjs`
  depending on the variant.
- `package.json` now has an `exports` field which instructs TypeScript how to
  resolve modules from either an ESM or CJS perspective.

In addition, the tests have been updated to use Vitest instead of Babel so that
they can read directly from the TypeScript configuration instead of having to
use an explicit transform step.

This commit is derived from the following PR on the Superstruct repo:
<ianstormtaylor#1211>

For more information on how TypeScript resolves modules, the "Modules Reference"
section of the TypeScript handbook is quite helpful, and in particular these
sections:

- <https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files>
- <https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext>
mcmire added a commit to MetaMask/superstruct that referenced this pull request Feb 9, 2024
Superstruct cannot be imported in a TypeScript project that uses a
`moduleResolution` of `node16` or `nodenext`. Any attempt to do so will result
in type errors such as the following:

```
node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 export * from './error';
                ~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './struct';
                ~~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:3:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

3 export * from './structs/coercions';
                ~~~~~~~~~~~~~~~~~~~~~

...
```

This is happening because although Superstruct is defined as an ESM library —
the `module` field in `package.json` is set to `true` — its published TypeScript
declaration files aren't ESM-compatible. This is due to the fact that imports
are missing extensions, which is a requirement for ESM.

At the same time, although Rollup is supposedly configured to emit both ESM- and
CJS-compatible files, TypeScript does not know how to use the CJS variants. The
`exports` field is the way to instruct TypeScript on how to do this, but
`package.json` is missing this field. This makes it impossible to use
Superstruct in a non-ESM library.

This commit fixes both of these issues by making the following changes:

- All imports in TypeScript files now use an extension of `.js`. This may seem
  like an odd choice, as there are no JavaScript files in the `src/` directory,
  but TypeScript doesn't resolve a module by trying to find a file with the
  given extension; it tries to find a declaration file that maps to the module
  path, based on a set of rules. Therefore, the file extension is really just a
  formality. In the end, Rollup is consolidating implementation files into one,
  so all of the imports will go away.
- Type declaration files have been split into ESM and CJS variants. Since Rollup
  generates ESM variants with an `.mjs` extension and CJS variants with a `.cjs`
  extension, imports in declaration files also use either `.mjs` or `.cjs`
  depending on the variant.
- `package.json` now has an `exports` field which instructs TypeScript how to
  resolve modules from either an ESM or CJS perspective.

In addition, the tests have been updated to use Vitest instead of Babel so that
they can read directly from the TypeScript configuration instead of having to
use an explicit transform step.

This commit is derived from the following PR on the Superstruct repo:
<ianstormtaylor#1211>

For more information on how TypeScript resolves modules, the "Modules Reference"
section of the TypeScript handbook is quite helpful, and in particular these
sections:

- <https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files>
- <https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext>
mcmire added a commit to MetaMask/superstruct that referenced this pull request Feb 9, 2024
Superstruct cannot be imported in a TypeScript project that uses a
`moduleResolution` of `node16` or `nodenext`. Any attempt to do so will result
in type errors such as the following:

```
node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 export * from './error';
                ~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './struct';
                ~~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:3:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

3 export * from './structs/coercions';
                ~~~~~~~~~~~~~~~~~~~~~

...
```

This is happening because although Superstruct is defined as an ESM library —
the `module` field in `package.json` is set to `true` — its published TypeScript
declaration files aren't ESM-compatible. This is due to the fact that imports
are missing extensions, which is a requirement for ESM.

At the same time, although Rollup is supposedly configured to emit both ESM- and
CJS-compatible files, TypeScript does not know how to use the CJS variants. The
`exports` field is the way to instruct TypeScript on how to do this, but
`package.json` is missing this field. This makes it impossible to use
Superstruct in a non-ESM library.

This commit fixes both of these issues by making the following changes:

- All imports in TypeScript files now use an extension of `.js`. This may seem
  like an odd choice, as there are no JavaScript files in the `src/` directory,
  but TypeScript doesn't resolve a module by trying to find a file with the
  given extension; it tries to find a declaration file that maps to the module
  path, based on a set of rules. Therefore, the file extension is really just a
  formality. In the end, Rollup is consolidating implementation files into one,
  so all of the imports will go away.
- Type declaration files have been split into ESM and CJS variants. Since Rollup
  generates ESM variants with an `.mjs` extension and CJS variants with a `.cjs`
  extension, imports in declaration files also use either `.mjs` or `.cjs`
  depending on the variant.
- `package.json` now has an `exports` field which instructs TypeScript how to
  resolve modules from either an ESM or CJS perspective.

In addition, the tests have been updated to use Vitest instead of Babel so that
they can read directly from the TypeScript configuration instead of having to
use an explicit transform step.

This commit is derived from the following PR on the Superstruct repo:
<ianstormtaylor#1211>

For more information on how TypeScript resolves modules, the "Modules Reference"
section of the TypeScript handbook is quite helpful, and in particular these
sections:

- <https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files>
- <https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext>
mcmire added a commit to MetaMask/superstruct that referenced this pull request Feb 9, 2024
Superstruct cannot be imported in a TypeScript project that uses a
`moduleResolution` of `node16` or `nodenext`. Any attempt to do so will result
in type errors such as the following:

```
node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 export * from './error';
                ~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './struct';
                ~~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:3:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

3 export * from './structs/coercions';
                ~~~~~~~~~~~~~~~~~~~~~

...
```

This is happening because although Superstruct is defined as an ESM library —
the `module` field in `package.json` is set to `true` — its published TypeScript
declaration files aren't ESM-compatible. This is due to the fact that imports
are missing extensions, which is a requirement for ESM.

At the same time, although Rollup is supposedly configured to emit both ESM- and
CJS-compatible files, TypeScript does not know how to use the CJS variants. The
`exports` field is the way to instruct TypeScript on how to do this, but
`package.json` is missing this field. This makes it impossible to use
Superstruct in a non-ESM library.

This commit fixes both of these issues by making the following changes:

- All imports in TypeScript files now use an extension of `.js`. This may seem
  like an odd choice, as there are no JavaScript files in the `src/` directory,
  but TypeScript doesn't resolve a module by trying to find a file with the
  given extension; it tries to find a declaration file that maps to the module
  path, based on a set of rules. Therefore, the file extension is really just a
  formality. In the end, Rollup is consolidating implementation files into one,
  so all of the imports will go away.
- Type declaration files have been split into ESM and CJS variants. Since Rollup
  generates ESM variants with an `.mjs` extension and CJS variants with a `.cjs`
  extension, imports in declaration files also use either `.mjs` or `.cjs`
  depending on the variant.
- `package.json` now has an `exports` field which instructs TypeScript how to
  resolve modules from either an ESM or CJS perspective.

In addition, the tests have been updated to use Vitest instead of Babel so that
they can read directly from the TypeScript configuration instead of having to
use an explicit transform step.

This commit is derived from the following PR on the Superstruct repo:
<ianstormtaylor#1211>

For more information on how TypeScript resolves modules, the "Modules Reference"
section of the TypeScript handbook is quite helpful, and in particular these
sections:

- <https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files>
- <https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext>
mcmire added a commit to MetaMask/superstruct that referenced this pull request Feb 9, 2024
Superstruct cannot be imported in a TypeScript project that uses a
`moduleResolution` of `node16` or `nodenext`. Any attempt to do so will result
in type errors such as the following:

```
node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 export * from './error';
                ~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './struct';
                ~~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:3:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

3 export * from './structs/coercions';
                ~~~~~~~~~~~~~~~~~~~~~

...
```

This is happening because although Superstruct is defined as an ESM library —
the `module` field in `package.json` is set to `true` — its published TypeScript
declaration files aren't ESM-compatible. This is due to the fact that imports
are missing extensions, which is a requirement for ESM.

At the same time, although Rollup is supposedly configured to emit both ESM- and
CJS-compatible files, TypeScript does not know how to use the CJS variants. The
`exports` field is the way to instruct TypeScript on how to do this, but
`package.json` is missing this field. This makes it impossible to use
Superstruct in a non-ESM library.

This commit fixes both of these issues by making the following changes:

- All imports in TypeScript files now use an extension of `.js`. This may seem
  like an odd choice, as there are no JavaScript files in the `src/` directory,
  but TypeScript doesn't resolve a module by trying to find a file with the
  given extension; it tries to find a declaration file that maps to the module
  path, based on a set of rules. Therefore, the file extension is really just a
  formality. In the end, Rollup is consolidating implementation files into one,
  so all of the imports will go away.
- Type declaration files have been split into ESM and CJS variants. Since Rollup
  generates ESM variants with an `.mjs` extension and CJS variants with a `.cjs`
  extension, imports in declaration files also use either `.mjs` or `.cjs`
  depending on the variant.
- `package.json` now has an `exports` field which instructs TypeScript how to
  resolve modules from either an ESM or CJS perspective.

In addition, the tests have been updated to use Vitest instead of Babel so that
they can read directly from the TypeScript configuration instead of having to
use an explicit transform step.

In conjunction with this change, the minimum Node version has been bumped to
16.x and the GitHub workflows have been updated to stop testing on Node 14.x.
14.x has reached EOL, the version of Vitest we're using isn't compatible with
this version, and a `target` of `esnext` is being used in the TypeScript config,
which require a JavaScript runtime that supports newer ES features such as
`??=`, so the minimum Node version is effectively 16.x anyway.

This commit is derived from the following PR on the Superstruct repo:
<ianstormtaylor#1211>

For more information on how TypeScript resolves modules, the "Modules Reference"
section of the TypeScript handbook is quite helpful, and in particular these
sections:

- <https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files>
- <https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext>
@MentalGear
Copy link

@ianstormtaylor Could you please review this PR - would allow for Sveltekit's most popular Form Validation Lib to use superstruct !

mcmire added a commit to MetaMask/superstruct that referenced this pull request Feb 29, 2024
## Description

Superstruct cannot be imported in a TypeScript project that uses a
`moduleResolution` of `node16` or `nodenext`. Any attempt to do so will
result in type errors such as the following:

```
node_modules/superstruct/dist/index.d.ts:1:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

1 export * from './error';
                ~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:2:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

2 export * from './struct';
                ~~~~~~~~~~

node_modules/superstruct/dist/index.d.ts:3:15 - error TS2834: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

3 export * from './structs/coercions';
                ~~~~~~~~~~~~~~~~~~~~~

...
```

This is happening because although Superstruct is defined as an ESM
library — the `module` field in `package.json` is set to `true` — its
published TypeScript declaration files aren't ESM-compatible. This is
due to the fact that imports are missing extensions, which is a
requirement for ESM.

At the same time, although Rollup is supposedly configured to emit both
ESM- and CJS-compatible files, TypeScript does not know how to use the
CJS variants. The `exports` field is the way to instruct TypeScript on
how to do this, but `package.json` is missing this field. This makes it
impossible to use Superstruct in a non-ESM library.

This commit fixes both of these issues by making the following changes:

- All imports in TypeScript files now use an extension of `.js`. This
may seem like an odd choice, as there are no JavaScript files in the
`src/` directory, but TypeScript doesn't resolve a module by trying to
find a file with the given extension; it tries to find a declaration
file that maps to the module path, based on a set of rules. Therefore,
the file extension is really just a formality. In the end, Rollup is
consolidating implementation files into one, so all of the imports will
go away.
- Type declaration files have been split into ESM and CJS variants.
Since Rollup generates ESM variants with an `.mjs` extension and CJS
variants with a `.cjs` extension, imports in declaration files also use
either `.mjs` or `.cjs` depending on the variant.
- `package.json` now has an `exports` field which instructs TypeScript
how to resolve modules from either an ESM or CJS perspective.

In addition, the tests have been updated to use Vitest instead of Babel
so that they can read directly from the TypeScript configuration instead
of having to use an explicit transform step.

## References

This commit is derived from the following PR on the Superstruct repo:
<ianstormtaylor#1211>

For more information on how TypeScript resolves modules, the "Modules
Reference" section of the TypeScript handbook is quite helpful, and in
particular these sections:

-
<https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files>
-
<https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext>
@arturmuller arturmuller mentioned this pull request Mar 11, 2024
@arturmuller
Copy link
Collaborator

Hi @ciscoheat — thank you so much for this PR — it is hugely appreciated! 🎉

Getting Superstruct to be compatible with NodeNext is my priority right now (I have recently taken over maintenance of Superstruct from Ian).

Given that Superstruct has quite a few downloads on npm, I would really like to make sure that versioning properly follows semver and we don't introduce some nasty surprises to unsuspecing users.

There are multiple interlocking issues going on here (node resolution, typescript config, mocha/testing) and I think we have to tackle them one at a time.

I made a new PR #1224 where I would love your input of the actual NodeNext compat. Importantly, if you want to try out the file-extensions solution to use superstruct with NodeNext now, you can install [email protected] prerelease version.

@ciscoheat
Copy link
Author

Hi Artur, thank you, I know I should have separated the two things, but they became intermingled during experimentation. I'm mostly concerned with NodeNext compat for Superforms, so I'll try the prerelease version and see how it goes! About #1226 I can separate the PR to only focus on vitest, but I'm afraid I cannot do much more with the time I have right now. Should I base it on the prerelease #1224?

@arturmuller
Copy link
Collaborator

Thank you for getting this off the ground @ciscoheat — I am closing this PR since we solved it in another PR, but your work on this is still super appreciated! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants