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

[JavaScript] - "node" module resolution #1760

Open
1 task done
nyan-left opened this issue Dec 2, 2024 · 11 comments · May be fixed by #1764
Open
1 task done

[JavaScript] - "node" module resolution #1760

nyan-left opened this issue Dec 2, 2024 · 11 comments · May be fixed by #1764

Comments

@nyan-left
Copy link

Report

  • I have searched existing issues and this is not a duplicate

Issues and Steps to Reproduce

When using Node module resolution, imports from subpaths like yoga-layout/load fail:

import { Overflow } from 'yoga-layout/load'  // fails

This occurs because the package.json uses exports that require bundler module resolution
(https://github.com/facebook/yoga/blob/main/javascript/package.json#L15):

"exports": {
    ".": "./src/index.ts",
    "./load": "./src/load.ts"
}

Many projects and libraries need to maintain compatibility with Node module resolution to support their users' existing codebases and build setups.
While direct application developers can potentially switch to "bundler" module resolution as a quick workaround, this becomes problematic for library authors building on top of yoga-layout. These library authors need to maintain the widest possible compatibility for their downstream users, making it difficult to require specific module resolution strategies.

Bundling yoga-layout as part of our library is not a viable solution as we want to leverage the existing npm ecosystem - allowing users to receive yoga-layout updates independently, maintain their own version management, and use standard module resolution patterns they're familiar with.

Expected Behavior

Imports from yoga-layout/load should resolve correctly when using node module resolution, ensuring broader compatibility across different projects and build configurations.

Actual Behavior

Get TypeScript error:

error TS2307: Cannot find module 'yoga-layout/load' or its corresponding type declarations.
@NickGerleman
Copy link
Contributor

node16 resolution should also work (with the caveats that option has more generally). That has been the resolution for Node 16 in @tsconfig/bases since May 2023 (which is a bit more recent than I expected).

Yoga took a dependence on ES modules for 3.0 to simplify the number of configurations, but that means type checker and bundler need to have some awareness of them. We removed the dependency on top-level await, because it was a bit too breaking, but the ecosystem is pretty increasingly depending on ES module semantics and resolution, so this is a place where projects really should try to put in the work to get a toolchain supporting them.

We export the main entrypoint outside package.json exports, but reaching into yoga-layout/dist/src/load would probably be the best way to get things working with old module resolution in the meantime.

@nyan-left
Copy link
Author

Thanks for the detailed response. I agree that upgrading to modern module resolution is ideal, though it's not always possible in current projects.

Currently we can't create libraries that work across both resolution strategies:

// Works with 'node' resolution, fails with 'bundler'
import { Overflow } from 'yoga-layout/dist/src/load'

// Works with 'bundler' resolution, fails with 'node'
import { Overflow } from 'yoga-layout/load'

Would something like this in the package.json help support both cases?

"exports": {
    ".": "./src/index.ts",
    "./load": "./src/load.ts",
    "./dist/src/load": "./dist/src/load.js",
    "./dist/src/load.js": "./dist/src/load.js"
}

@NickGerleman
Copy link
Contributor

I think I’d be okay with going the other way, of adding a “/load” entry point file that just lives at the root for those not able to use ES resolution or top level await. So long as it’s able to resolve to the right place, after the transformation we do when shipping the source (yarn prepack triggers this).

@nyan-left
Copy link
Author

Works for me. I'll make a PR at some point next week.

@nyan-left
Copy link
Author

nyan-left commented Dec 6, 2024

@NickGerleman For further compatibility, what are your thoughts on switching the import extensions from .ts to .js, or using the new --rewriteRelativeImportExtensions to transform them? Currently the imports within the declaration files all have .ts extensions.

https://devblogs.microsoft.com/typescript/announcing-typescript-5-7/#path-rewriting-for-relative-paths

@NickGerleman
Copy link
Contributor

We rewrite .ts to .js in the source when we run the Babel transform before packing: https://github.com/facebook/yoga/blob/main/javascript/babel.config.cjs#L26

The packing step will also replace .ts with .js for entrypoints:

packageJson.main = packageJson.main.replace(

The declaration files do still have a .ts extension, but actually resolve to the .d.ts files, and are just used for type imports.

The original source files pre-transform are also still there, without the rewriting, but they should never be directly imported. They are more useful just for source maps pointing to original source if debugging the released package.

@nyan-left
Copy link
Author

This approach doesn't work in TypeScript versions prior to 5.0 - you'll get TS2691 errors complaining that import paths cannot end with .ts extensions in declaration files, even though these are just type imports. For example:

error TS2691: An import path cannot end with a '.ts' extension. Consider importing './wrapAssembly.js' instead.

@NickGerleman
Copy link
Contributor

NickGerleman commented Dec 6, 2024

Oof, that's fun... It looks like we never tested this with a version before 5.0.

Versions before 5.0 are out of support on DefinitelyTyped, but rewriting the imports in declarations is probably more correct, and pretty innocuous (the only case we use tsc generated output is for these declarations). I have no objections to adding that (and bumping TS version in the repo to support it).

@nyan-left
Copy link
Author

nyan-left commented Dec 6, 2024

So it looks like declaration extensions don't get rewritten microsoft/TypeScript#59767 (comment) 🙃

For now, I see two paths:

  • We tweak .ts to .js manually in TypeScript source files.
  • We do something similar to prepack-package-json, but this seems a little convoluted.

@NickGerleman
Copy link
Contributor

Wouldn’t the first option break type-checking during development?

If TypeScript team says that .d.ts files don’t get rewritten because they are allowed to import via .ts, and the supported (last two years) of TS allow this, it might be better not to fuss with too much.

@nyan-left nyan-left linked a pull request Dec 6, 2024 that will close this issue
@nyan-left
Copy link
Author

No, types are maintained. Try this branch - #1764

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 a pull request may close this issue.

2 participants