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

Add gentypeconfig.moduleResolution options: node16 and bundler for better interop with TS+ESM projects #6182

Merged
merged 18 commits into from
Apr 24, 2023

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Apr 20, 2023

Resolves #6003

This introduces a new gentype option moduleResolution. This makes it able to customize the artifacts module imports strategy customizable, so it provides compatibility with the compilerOptions.moduleResolution setting in TypeScript projects.

With moduleResolution:

  • node (default): Drop extensions in import paths. This doesn't change any behavior.
  • node16: Use TS output's extension. This provides compatibility with projects using moduleResolution: node16 and ESM.
  • bundler: Use TS input's extension. This provides compatibility with projects using moduleResolution: bundler and ESM. This also requires TS v5.0+ and compilerOptions.allowImportingTsExtensions to true

@cometkim cometkim changed the title Gh 6003 Add gentypeconfig.moduleResolution options: node16 and bundler for better interop with TS Apr 20, 2023
@cometkim cometkim changed the title Add gentypeconfig.moduleResolution options: node16 and bundler for better interop with TS Add gentypeconfig.moduleResolution options: node16 and bundler for better interop with TS+ESM projects Apr 23, 2023
@cometkim cometkim marked this pull request as ready for review April 23, 2023 11:55
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a first quick look, and everything looks fine. Will read in more detail later.
There are questions about adding new tests.

Makefile Outdated Show resolved Hide resolved
jscomp/gentype/GenTypeConfig.ml Outdated Show resolved Hide resolved
@cristianoc
Copy link
Collaborator

Also, the changelog needs updating.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of complete review.
Left some comments, but it all seems good.
Todo still remove the additional projects unless they bring some value in which case they can stay.

CHANGELOG.md Outdated
@@ -15,6 +15,10 @@
#### :rocket: Main New Feature

- Add support for Dynamic import. https://github.com/rescript-lang/rescript-compiler/pull/5703
- GenType: Add `moduleResolution` option to customize extensions on emitted import statements. This helps to adjust output compatibility with TypeScript projects using ESM. https://github.com/rescript-lang/rescript-compiler/pull/6182
- `node` (default): Drop extensions.
- `node16`: Use TS output's extensions (e.g. `.gen.js`). Make it ESM-compatible.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ".e.g"? Isn't it exactly .gen.js?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhhh if I'm guessing right, the output of GenType is always .gen.ts or .gen.tsx and can't be customized with a different extension?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let inputFileSuffix ~(config : Config.t) =
  match config.generatedFileExtension with
  | Some s when Filename.extension s <> "" (* double extension  *) -> s
  | _ -> generatedFilesExtension ~config ^ ".tsx"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this code, I don't understand the naming of the functions:

let generatedModuleExtension ~(config : Config.t) =
  match config.moduleResolution with
  | Node -> generatedFilesExtension ~config
  | Node16 -> outputFileSuffix ~config
  | Bundler -> inputFileSuffix ~config

One is "input" one is "output" etc. Seems confusing.
Is there a more intuitive renaming?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a multiplication of concepts here:

  • node, node16, bundler
  • input, output
  • .js, .tsx

Feels like one has to keep all those in their head to understand what this does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the "e.g." I think it should be removed. If it's not always exactly that, say what config determines the extension and how, instead of "e.g.".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have the right sentences to use for the documentation website already.

Copy link
Member Author

@cometkim cometkim Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note:

TS bundler moduleResolution means "hybrid". Whether it has no extension, .ts or .js, its resolution depends on the user's build tool (bundler)

Compatible with most bundlers, the most naive way to implement this is to specify the input filename exactly. Therefore, no implicit extension inference takes place.

See microsoft/TypeScript#51669

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is, the behaviour of genType should be specified precisely. That's all.

jscomp/gentype/GenTypeConfig.ml Show resolved Hide resolved
jscomp/gentype/GenTypeConfig.ml Show resolved Hide resolved
jscomp/gentype/GenTypeMain.ml Show resolved Hide resolved
jscomp/gentype/ModuleExtension.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ready to merge.
Anything missing?

@cometkim
Copy link
Member Author

Ready to merge

@cristianoc
Copy link
Collaborator

Ready to merge

Fantastic.
Awesome contribution.

@cristianoc cristianoc merged commit 39fdfe8 into rescript-lang:master Apr 24, 2023
@cometkim cometkim deleted the gh-6003 branch April 24, 2023 14:17
@cometkim
Copy link
Member Author

Thank you for the detailed review 😁

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.

Feature request: customize removal of extension on es6 imports to better support TypeScript/deno environments
2 participants