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

NodeNext Compat #1224

Merged
merged 7 commits into from
Jun 20, 2024
Merged

NodeNext Compat #1224

merged 7 commits into from
Jun 20, 2024

Conversation

arturmuller
Copy link
Collaborator

@arturmuller arturmuller commented Mar 11, 2024

👉 This PR can be tested via a pre-release version of Superstruct: npm i [email protected].

The goal of this PR is to fix Superstruct's compatibility with TypeScript's NodeNext module / module resolution settings. The simplest fix I could think of (and which was also proposed in #1211 by @ciscoheat) is to add file extensions to imports. This is currently in the PR, however, this presents several issues:

  1. It breaks tests. Test are currently written in Mocha, which is really starting to show its age — it is quite hard to figure out how to make things play nice with the rest of the tooling. @ciscoheat proposed a switch to Vitest which I am 100% in favour of, but I think we need to make the switch a bit more thoughtfully: a) Make sure the tests are actually written in a more standard Vitest way and that all test are passing.
  2. We have to actually use the file extensions, which — in my experience — is rather confusing to new contributors. The extension has to be .js not .ts (since TS doesn't rewrite paths), and we lose directory import. Bottom line, this feels like a non-ideal way to do this and other libs manage in a more elegant way, so we investigate how this can be done.
  3. There are many environments where Superstruct can run, and any changes to module resolution should be either thoroughly tested (at least manually), or marked as a breaking change to make sure we don't break suddenly people's builds. I would really hate to drop the whole TS/Node/CommonJS/ESM mess on unsuspecting users.

This fixes compat with TypeScript's NodeNext module resolution.
@arturmuller arturmuller marked this pull request as draft March 11, 2024 12:50
@ciscoheat
Copy link

I'm happy to say that this worked perfectly with Superforms, so I'm ready to add it to the library. Will it be a big change for the next official version, or can I add it already with the pre-release version?

@ciscoheat
Copy link

Hello again @arturmuller, any progress on this one? It would be very nice to support another library for Superforms.

@Jakeii
Copy link

Jakeii commented May 3, 2024

I suggest adding tsc-alias to the build process, you can leave the extensions out of the ts files, and just add the tsc-alias command after the build command.

with

  "tsc-alias": {
    "resolveFullPaths": true
  }

in the tsconfig.

it'll add extensions to all the paths in the type definitions.

PR: #1232

@Jakeii
Copy link

Jakeii commented May 10, 2024

I suggest adding tsc-alias to the build process, you can leave the extensions out of the ts files, and just add the tsc-alias command after the build command.

with

  "tsc-alias": {
    "resolveFullPaths": true
  }

in the tsconfig.

it'll add extensions to all the paths in the type definitions.

PR: #1232

Hey @arturmuller what do you think?

@yeoffrey
Copy link
Contributor

yeoffrey commented Jun 5, 2024

This could be rebased onto main now that the tests are running on something newer!

It seems like the prettier API has changed and it is no longer possible to combine --list-different and --write.
@arturmuller arturmuller marked this pull request as ready for review June 20, 2024 13:23
@arturmuller arturmuller merged commit 3fe2139 into main Jun 20, 2024
4 checks passed
@arturmuller arturmuller deleted the node-next-compat branch June 20, 2024 13:24
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.

4 participants