DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

[JavaScript] - "node" module resolution #1760

New Issue
Closed
opened 2024-12-02 15:33:25 -08:00 by nyan-left · 12 comments
nyan-left commented 2024-12-02 15:33:25 -08:00 (Migrated from github.com)
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.
# Report - [x] 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: ```ts 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): ```json "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 commented 2024-12-02 18:00:17 -08:00 (Migrated from github.com)
Copy Link

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.

`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](https://github.com/tsconfig/bases/commit/47d3c29c650091c9e2eaad27bd1133db63f0fd22) (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 commented 2024-12-02 19:46:15 -08:00 (Migrated from github.com)
Copy Link

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"
}
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: ```typescript // 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? ```json "exports": { ".": "./src/index.ts", "./load": "./src/load.ts", "./dist/src/load": "./dist/src/load.js", "./dist/src/load.js": "./dist/src/load.js" } ```
NickGerleman commented 2024-12-04 18:03:45 -08:00 (Migrated from github.com)
Copy Link

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).

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 commented 2024-12-05 10:34:29 -08:00 (Migrated from github.com)
Copy Link

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

Works for me. I'll make a PR at some point next week.
nyan-left commented 2024-12-06 04:11:22 -08:00 (Migrated from github.com)
Copy Link

@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 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 commented 2024-12-06 04:25:42 -08:00 (Migrated from github.com)
Copy Link

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: 050ac8a413/javascript/just.config.cjs (L61)

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.

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: https://github.com/facebook/yoga/blob/050ac8a4132fa5c888eb7c3518ea8c95dc5f6bec/javascript/just.config.cjs#L61 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 commented 2024-12-06 04:32:45 -08:00 (Migrated from github.com)
Copy Link

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.
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 commented 2024-12-06 04:50:36 -08:00 (Migrated from github.com)
Copy Link

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).

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](https://github.com/DefinitelyTyped/DefinitelyTyped?tab=readme-ov-file#support-window), 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 commented 2024-12-06 06:06:57 -08:00 (Migrated from github.com)
Copy Link

So it looks like declaration extensions don't get rewritten https://github.com/microsoft/TypeScript/pull/59767#issuecomment-2389761587 🙃

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.
So it looks like declaration extensions don't get rewritten https://github.com/microsoft/TypeScript/pull/59767#issuecomment-2389761587 🙃 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 commented 2024-12-06 06:24:41 -08:00 (Migrated from github.com)
Copy Link

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.

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 commented 2024-12-06 06:33:08 -08:00 (Migrated from github.com)
Copy Link

No, types are maintained. Try this branch - https://github.com/facebook/yoga/pull/1764

No, types are maintained. Try this branch - https://github.com/facebook/yoga/pull/1764
nyan-left commented 2024-12-26 20:42:17 -08:00 (Migrated from github.com)
Copy Link

Closing - support not planned https://github.com/facebook/yoga/pull/1764#issuecomment-2561137878

Closing - support not planned https://github.com/facebook/yoga/pull/1764#issuecomment-2561137878
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1760
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?