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

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

Closed
chrisirhc opened this issue Feb 12, 2023 · 15 comments · Fixed by #6182

Comments

@chrisirhc
Copy link

chrisirhc commented Feb 12, 2023

Looks like gentype only supports .bs.js extension but then proceeds to remove the .js in the import statements.
Examples missing extension:

import {make as makeNotChecked} from './hookExample';

This behavior relies on non-standard import behavior. According to MDN ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#module-name ) ,

The module to import from. The evaluation of the specifier is host-specified. This is often a relative or absolute URL to the .js file containing the module. In Node, extension-less imports often refer to packages in node_modules. Certain bundlers may permit importing files without extensions; check your environment. Only single quoted and double quoted Strings are allowed.

If the .js isn't removed, the TS code and JS code could be compatible with Deno and potentially other bundlers/environments.

Looks like this behavior is also present in the main compiler and on every .bs.js file. Wonder if the community would consider this change interesting/useful to open up to more environments perhaps like Deno or Bun etc.

Related to #4965 #5530

@chrisirhc chrisirhc changed the title Feature request: customize extension on es6 imports in gentype Feature request: customize removal of extension on es6 imports Feb 12, 2023
@chrisirhc chrisirhc changed the title Feature request: customize removal of extension on es6 imports Feature request: customize removal of extension on es6 imports to better support TypeScript environments Feb 25, 2023
@chrisirhc chrisirhc changed the title Feature request: customize removal of extension on es6 imports to better support TypeScript environments Feature request: customize removal of extension on es6 imports to better support TypeScript/deno environments Feb 25, 2023
@cristianoc
Copy link
Collaborator

This is what happens if you add the extension:

import {make as makeNotChecked} from './hookExample.tsx';

then run tsc:

% npx tsc
src/JSXV4.gen.tsx:5:38 - error TS2691: An import path cannot end with a '.tsx' extension. Consider importing './hookExample' instead.

5 import {make as makeNotChecked} from './hookExample.tsx';
                                       ~~~~~~~~~~~~~~~~~~~


Found 1 error.

@chrisirhc
Copy link
Author

Looks like this extension enforcement was a constraint in TypeScript but no longer as of a month ago:
microsoft/TypeScript#27481 (comment)

@cristianoc
Copy link
Collaborator

We can't assume people have the latest TS, and have configured it in that specific way.

@chrisirhc
Copy link
Author

You’re right, we can’t. But we can assume that deno users are using that configuration. The current configuration breaks deno usage completely. I’m not asking for a new default, but that this behavior be configurable.

@cristianoc
Copy link
Collaborator

I'd like to learn more about that.
Perhaps the example mentioned is not the intended one. Notice this can be used in the source file:

@genType.import("./hookExample.tsx")
@react.component
external make: (
...

and produces an import to `.tsx.

I guess you're referring to other examples.
It might be useful to start with a couple of small self-contained examples that illustrate the issue.

@cometkim
Copy link
Member

Adding a module resolution mode may be reasonable. TypeScript requires different extensions depending on the tsconfig.json

https://www.typescriptlang.org/tsconfig#moduleResolution

  • { "moduleResolution": "Node" }: It's for Node's node_modules resolution (basically require.resolve). When importing a local module from a relative path, there must be no extension.
  • { "moduleResolution": "Node16" } or { "moduleResolution": "NodeNext" }: For compatibilities with ESM. When importing a local module from a relative path, there must be a extension in .js (or .cjs/.mjs), not .ts
  • { "moduleResolution": "Bundler" }: From TypeScript v5, it allows extensions of all formats commonly supported by bundler. (including no extensions). This also available for Deno or Bun which use bundler internally.

@cristianoc
Copy link
Collaborator

What's a concrete proposal for module resolution mode? How would the setting look like and what would the effect be?

@cometkim
Copy link
Member

What's a concrete proposal for module resolution mode? How would the setting look like and what would the effect be?

Usually the setting is only needed in gentypeconfig and modifies the way to import statments are printed.

Some given code like:

@genType
let makeOther: () => Other.t = { ... }

would generate like this:

import * as Module_BS__Es6Import from './Module.bs';

import type {t as Other_t} from './Other.gen';

Assuming this follows the default settings:

{
  "gentypeconfig": {
    "moduleResolution": "node" // default, identical to current behavior and TypeScript's default.
  }
}

(Option names intentionally follows the naming convention of tsconfig.json's)

In a TypeScript project with the same setup, this works well.

If user changes the setting to:

{
  "gentypeconfig": {
    "moduleResolution": "node16"
  }
}

should generates:

import * as Module_BS__Es6Import from './Module.bs.js'; // extension required for ESM compatibility

import type {t as Other_t} from './Other.gen.js'; // extension here should follows the TS output's

and

{
  "gentypeconfig": {
    "moduleResolution": "bundler"
  }
}

should generates:

import * as Module_BS__Es6Import from './Module.bs.js';

import type {t as Other_t} from './Other.gen.ts'; // TS extension for most bundlers, Deno, Bun, etc

@cristianoc
Copy link
Collaborator

Does one of these names "bundler" etc indicate a specific extension? Is it better to specify by name or by extension?

@cristianoc
Copy link
Collaborator

In general, this should be pretty easy to add.

@cristianoc
Copy link
Collaborator

PRs accepted. This could be a good opportunity to check that the genType code is easy to modify. And if not, clean up parts of it in the process based on feedback. Generally, it should be quite approachable.

@cometkim
Copy link
Member

cometkim commented Apr 20, 2023

Does one of these names "bundler" etc indicate a specific extension? Is it better to specify by name or by extension?

IMO it's easy to understand when matched with the tsconfig. Since It is intended for interoperability with the TypeScript projects.

Emitting with full extension is enough for JS/Flow. And user can customize it's own resolution for Flow by the flowconfig anyway.

@cristianoc
Copy link
Collaborator

cristianoc commented Apr 20, 2023

If tsconfig uses those names then it's perfect.
Notice gentype is now part of the compiler repo, and there is no support for Flow anymore. So one can focus on give better support for TS.

@cometkim
Copy link
Member

and there is no support for Flow anymore.

Oh, I didn't notice that. bit sad to me 🥲

@cristianoc
Copy link
Collaborator

There are some rescript+flow users still, but those we know about have not updated the compiler version in several years, so they should not be affected.

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