Skip to content

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

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

Merged
merged 18 commits into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

- `bundler`: Use TS input's extensions (e.g. `.gen.tsx`). Make it ESM-compatible.

#### :boom: Breaking Change

Expand Down
22 changes: 7 additions & 15 deletions jscomp/gentype/EmitType.ml
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,6 @@ let fileHeader ~sourceFile =
~lines:["TypeScript file generated from " ^ sourceFile ^ " by genType."]
^ "/* eslint-disable import/first */\n\n"

let generatedFilesExtension ~(config : Config.t) =
match config.generatedFileExtension with
| Some s ->
(* from .foo.bar to .foo *)
Filename.remove_extension s
| None -> ".gen"

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

let generatedModuleExtension ~config = generatedFilesExtension ~config
let shimExtension = ".shim.ts"

let interfaceName ~(config : Config.t) name =
match config.exportInterfaces with
| true -> "I" ^ name
Expand Down Expand Up @@ -387,6 +372,13 @@ let emitRequire ~importedValueOrComponent ~early ~emitters ~(config : Config.t)
| true -> "// tslint:disable-next-line:no-var-requires\n"
| false -> "// @ts-ignore: Implicit any on import\n"
in
let importPath =
match config.moduleResolution with
| Node ->
importPath
|> ImportPath.chopExtensionSafe (* for backward compatibility *)
| _ -> importPath
in
match config.module_ with
| ES6 when not importedValueOrComponent ->
let moduleNameString = ModuleName.toString moduleName in
Expand Down
25 changes: 22 additions & 3 deletions jscomp/gentype/GenTypeConfig.ml
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
module ModuleNameMap = Map.Make (ModuleName)

type module_ = CommonJS | ES6

(** Compatibility for `compilerOptions.moduleResolution` in TypeScript projects. *)
type moduleResolution =
| Node (** should drop extension on import statements *)
| Node16
(** should use TS output's extension (e.g. `.gen.js`) on import statements *)
| Bundler
(** should use TS input's extension (e.g. `.gen.tsx`) on import statements *)

type bsVersion = int * int * int

type t = {
Expand All @@ -13,6 +22,7 @@ type t = {
exportInterfaces: bool;
generatedFileExtension: string option;
module_: module_;
moduleResolution: moduleResolution;
namespace: string option;
platformLib: string;
mutable projectRoot: string;
Expand All @@ -32,12 +42,13 @@ let default =
exportInterfaces = false;
generatedFileExtension = None;
module_ = ES6;
moduleResolution = Node;
namespace = None;
platformLib = "";
projectRoot = "";
shimsMap = ModuleNameMap.empty;
sources = None;
suffix = "";
suffix = ".bs.js";
}

let bsPlatformLib ~config =
Expand Down Expand Up @@ -112,6 +123,7 @@ let readConfig ~getBsConfigFile ~namespace =
in
let parseConfig ~bsconf ~gtconf =
let moduleString = gtconf |> getStringOption "module" in
let moduleResolutionString = gtconf |> getStringOption "moduleResolution" in
let exportInterfacesBool = gtconf |> getBool "exportInterfaces" in
let generatedFileExtensionStringOption =
gtconf |> getStringOption "generatedFileExtension"
Expand Down Expand Up @@ -143,6 +155,13 @@ let readConfig ~getBsConfigFile ~namespace =
| None, Some ("es6" | "es6-global") -> ES6
| _ -> default.module_
in
let moduleResolution =
match moduleResolutionString with
| Some "node" -> Node
| Some "node16" -> Node16
| Some "bundler" -> Bundler
| _ -> default.moduleResolution
in
let exportInterfaces =
match exportInterfacesBool with
| None -> default.exportInterfaces
Expand Down Expand Up @@ -171,9 +190,8 @@ let readConfig ~getBsConfigFile ~namespace =
in
let suffix =
match bsconf |> getStringOption "suffix" with
| Some ".bs.js" -> ".bs"
| Some s -> s
| _ -> ".bs"
| _ -> default.suffix
in
let bsDependencies =
match bsconf |> getOpt "bs-dependencies" with
Expand Down Expand Up @@ -204,6 +222,7 @@ let readConfig ~getBsConfigFile ~namespace =
exportInterfaces;
generatedFileExtension;
module_;
moduleResolution;
namespace;
platformLib;
projectRoot;
Expand Down
4 changes: 2 additions & 2 deletions jscomp/gentype/GenTypeMain.ml
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ let processCmtFile cmt =
let fileName = cmt |> Paths.getModuleName in
let isInterface = Filename.check_suffix cmtFile ".cmti" in
let resolver =
ModuleResolver.createLazyResolver ~config
~extensions:[".res"; EmitType.shimExtension] ~excludeFile:(fun fname ->
ModuleResolver.createLazyResolver ~config ~extensions:[".res"; ".shim.ts"]
~excludeFile:(fun fname ->
fname = "React.res" || fname = "ReasonReact.res")
in
let inputCMT, hasGenTypeAnnotations =
Expand Down
16 changes: 7 additions & 9 deletions jscomp/gentype/ImportPath.ml
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,17 @@ let fromModule ~dir ~importExtension moduleName =

let fromStringUnsafe s = ("", s)

let chopExtensionSafe s =
try s |> Filename.chop_extension with Invalid_argument _ -> s
let chopExtensionSafe (dir, s) =
try (dir, s |> Filename.chop_extension) with Invalid_argument _ -> (dir, s)

let dump (dir, s) = NodeFilename.concat dir s

let toCmt ~(config : Config.t) ~outputFileRelative (dir, s) =
let open Filename in
concat
(outputFileRelative |> dirname)
(((dir, s |> chopExtensionSafe) |> dump)
^ (match config.namespace with
| None -> ""
| Some name -> "-" ^ name)
^ ".cmt")
concat (outputFileRelative |> dirname) ((dir, s) |> chopExtensionSafe |> dump)
^ (match config.namespace with
| None -> ""
| Some name -> "-" ^ name)
^ ".cmt"

let emit (dir, s) = (dir, s) |> dump
1 change: 1 addition & 0 deletions jscomp/gentype/ImportPath.mli
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ open GenTypeCommon
type t

val bsCurryPath : config:Config.t -> t
val chopExtensionSafe : t -> t
val dump : t -> string
val emit : t -> string
val fromModule : dir:string -> importExtension:string -> ModuleName.t -> t
Expand Down
28 changes: 28 additions & 0 deletions jscomp/gentype/ModuleExtension.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
open GenTypeCommon

let shimExtension ~(config : Config.t) =
match config.moduleResolution with
| Node -> ".shim"
| Node16 -> ".shim.js"
| Bundler -> ".shim.ts"

let generatedFilesExtension ~(config : Config.t) =
match config.generatedFileExtension with
| Some s ->
(* from .foo.bar to .foo *)
Filename.remove_extension s
| None -> ".gen"

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

let outputFileSuffix ~(config : Config.t) =
generatedFilesExtension ~config ^ ".js"

let generatedModuleExtension ~(config : Config.t) =
match config.moduleResolution with
| Node -> generatedFilesExtension ~config
| Node16 -> outputFileSuffix ~config
| Bundler -> inputFileSuffix ~config
7 changes: 4 additions & 3 deletions jscomp/gentype/ModuleResolver.ml
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ let resolveGeneratedModule ~config ~outputFileRelative ~resolver moduleName =
(moduleName |> ModuleName.toString);
let importPath =
resolveModule ~config
~importExtension:(EmitType.generatedModuleExtension ~config)
~importExtension:(ModuleExtension.generatedModuleExtension ~config)
~outputFileRelative ~resolver ~useBsDependencies:true moduleName
in
if !Debug.moduleResolution then
Expand All @@ -272,9 +272,10 @@ let importPathForReasonModuleName ~(config : Config.t) ~outputFileRelative
| shimModuleName ->
if !Debug.moduleResolution then
Log_.item "ShimModuleName: %s\n" (shimModuleName |> ModuleName.toString);
let importExtension = ModuleExtension.shimExtension ~config in
let importPath =
resolveModule ~config ~importExtension:".shim" ~outputFileRelative
~resolver ~useBsDependencies:false shimModuleName
resolveModule ~config ~importExtension ~outputFileRelative ~resolver
~useBsDependencies:false shimModuleName
in
if !Debug.moduleResolution then
Log_.item "Import Path: %s\n" (importPath |> ImportPath.dump);
Expand Down
2 changes: 1 addition & 1 deletion jscomp/gentype/Paths.ml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ let findNameSpace cmt =
|> keepAfterDash

let getOutputFileRelative ~config cmt =
(cmt |> handleNamespace) ^ EmitType.outputFileSuffix ~config
(cmt |> handleNamespace) ^ ModuleExtension.inputFileSuffix ~config

let getOutputFile ~(config : Config.t) cmt =
Filename.concat config.projectRoot (getOutputFileRelative ~config cmt)
Expand Down
50 changes: 25 additions & 25 deletions jscomp/gentype_tests/typescript-react-example/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions jscomp/gentype_tests/typescript-react-example/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
"tsc": "tsc -p tsconfig.json"
},
"devDependencies": {
"@types/node": "^13.13.4",
"@types/react-dom": "^16.9.7",
"@types/node": "^18.15.12",
"@types/react-dom": "^18.0.11",
"rescript": "file:../../..",
"typescript": "3.9.2"
"typescript": "^5.0.4"
}
}
17 changes: 17 additions & 0 deletions jscomp/gentype_tests/typescript-res-bundler/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# See https://help.github.com/ignore-files/ for more about ignoring files.

# dependencies
/node_modules


# misc
/lib
.DS_Store
.env.local
.env.development.local
.env.test.local
.env.production.local

npm-debug.log*
yarn-debug.log*
yarn-error.log*
15 changes: 15 additions & 0 deletions jscomp/gentype_tests/typescript-res-bundler/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
SHELL = /bin/bash

node_modules/.bin/tsc:
npm install

test: node_modules/.bin/tsc
npm run build
npm run tsc

clean:
rm -rf node_modules lib src/*.bs.js src/*.gen.tsx

.DEFAULT_GOAL := test

.PHONY: clean test
Loading