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

v13 is not compatible with "moduleResolution": "node12" of Typescript 4.5 #43886

Open
JounQin opened this issue Oct 19, 2021 · 6 comments
Open
Assignees
Labels
area: packaging Issues related to Angular's creation of npm packages P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone

Comments

@JounQin
Copy link
Contributor

JounQin commented Oct 19, 2021

Which @angular/* package(s) are the source of the bug?

compiler

Is this a regression?

No

Description

.js suffix is required for native ESM on Node 12+ for all relative or absolute import statements

https://github.com/angular/angular/blob/master/packages/compiler/index.ts#L14

Please provide a link to a minimal reproduction of the bug

microsoft/TypeScript#46408 (comment)

Please provide the exception or error you saw

src/index.ts:1:15 - error TS2305: Module '"@angular/compiler"' has no exported member 'ParsedTemplate'.

1 import type { ParsedTemplate } from '@angular/compiler'
                ~~~~~~~~~~~~~~

src/index.ts:1:37 - error TS1471: Module '@angular/compiler' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

1 import type { ParsedTemplate } from '@angular/compiler'
                                      ~~~~~~~~~~~~~~~~~~~

src/index.ts:2:30 - error TS7016: Could not find a declaration file for module 'synckit'. '/Users/JounQin/Workspaces/Local/test/node_modules/synckit/lib/index.cjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/synckit` if it exists or add a new declaration (.d.ts) file containing `declare module 'synckit';`

2 import { createSyncFn } from 'synckit'
                               ~~~~~~~~~

src/worker.ts:1:15 - error TS2305: Module '"@angular/compiler"' has no exported member 'ParsedTemplate'.

1 import type { ParsedTemplate } from '@angular/compiler'
                ~~~~~~~~~~~~~~

src/worker.ts:1:37 - error TS1471: Module '@angular/compiler' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

1 import type { ParsedTemplate } from '@angular/compiler'
                                      ~~~~~~~~~~~~~~~~~~~

src/worker.ts:2:29 - error TS7016: Could not find a declaration file for module 'synckit'. '/Users/JounQin/Workspaces/Local/test/node_modules/synckit/lib/index.cjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/synckit` if it exists or add a new declaration (.d.ts) file containing `declare module 'synckit';`

2 import { runAsWorker } from 'synckit'
                              ~~~~~~~~~

src/worker.ts:5:11 - error TS2339: Property 'parseTemplate' does not exist on type 'typeof import("/Users/JounQin/Workspaces/Local/test/node_modules/@angular/compiler/index")'.

5   const { parseTemplate } = await import('@angular/compiler')
            ~~~~~~~~~~~~~


Found 7 errors.

Please provide the environment you discovered this bug in

@angular/compiler            13.0.0-rc.0

Anything else?

No response

@JounQin
Copy link
Contributor Author

JounQin commented Oct 19, 2021

Or native ESM actually.

@AndrewKushnir AndrewKushnir added the area: compiler Issues related to `ngc`, Angular's template compiler label Oct 19, 2021
@ngbot ngbot bot added this to the needsTriage milestone Oct 19, 2021
@JoostK
Copy link
Member

JoostK commented Oct 21, 2021

Using TypeScript 4.5 is not yet supported and we haven't verified how it works in any way yet, especially not in combination with its node12 module resolution mode. We are aware that the published ESM bundles are missing the file extension; we're relying on bundlers to use these modules. We were not aware that the .d.ts files have the same restriction, although that sort of makes sense now that you reported it. Our current thinking is that we may end up flattening the .d.ts files to get rid of the imports entirely, while still supporting pre-node12 module resolution.

Interestingly, the TypeScript team is considering making node12 module resolution support experimental in TS4.5 per microsoft/TypeScript#46452. That issue also mentions that (some) bundlers do not support imports with .js file extensions, which is what TS would require us to write.

@JoostK JoostK added area: packaging Issues related to Angular's creation of npm packages and removed area: compiler Issues related to `ngc`, Angular's template compiler labels Oct 21, 2021
@JoostK
Copy link
Member

JoostK commented Oct 21, 2021

That's only because the JS bundles in @angular/compiler have already been bundled! But the .d.ts files have not.

@JounQin
Copy link
Contributor Author

JounQin commented Oct 21, 2021

That's only because the JS bundles in @angular/compiler have already been bundled! But the .d.ts files have not.

Yeah, sorry, I've just realized that. 😅

@devversion devversion self-assigned this Oct 21, 2021
@devversion devversion added the P4 A relatively minor issue that is not relevant to core functions label Oct 21, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Oct 21, 2021
@devversion devversion added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed P4 A relatively minor issue that is not relevant to core functions labels Nov 20, 2021
@crisbeto
Copy link
Member

I explored this in #45775 which revealed the following error when bundling:

[!] Error: Could not resolve './utils/index.js' from bazel-out/k8-fastbuild-ST-2e5f3376adb5/bin/packages/localize/src/translate.mjs
Error: Could not resolve './utils/index.js' from bazel-out/k8-fastbuild-ST-2e5f3376adb5/bin/packages/localize/src/translate.mjs
    at error (/b/f/w/bazel-out/k8-opt-exec-B27D3D4C/bin/packages/bazel/src/ng_package/rollup_for_ng_package.sh.runfiles/npm/node_modules/rollup/dist/shared/rollup.js:198:30)
    at ModuleLoader.handleResolveId (/b/f/w/bazel-out/k8-opt-exec-B27D3D4C/bin/packages/bazel/src/ng_package/rollup_for_ng_package.sh.runfiles/npm/node_modules/rollup/dist/shared/rollup.js:22461:24)
    at /b/f/w/bazel-out/k8-opt-exec-B27D3D4C/bin/packages/bazel/src/ng_package/rollup_for_ng_package.sh.runfiles/npm/node_modules/rollup/dist/shared/rollup.js:22424:26

After talking to @devversion, the problem appears to be that Bazel emits .mjs files while the imports are referring to .js. Getting around this is somewhat tricky with our current build setup.

For future reference, I wrote this script that adds the .js extensions automatically: https://github.com/crisbeto/angular/blob/bf98b3a161989344e59af3cdbef98ded3fe53f3c/change-imports.js

@devversion
Copy link
Member

Yes, to capture some more details here: Long-term the best solution is to actually use the explicit extension (like @crisbeto built a script for). For now, all of our packages should be compatible with strict ESM at runtime and both with TypeScript's new resolution because runtime code, as well as types are bundled (mitigating the need of an explicit extension). This is now enforced with #45405.

Its worth noting that compiler-cli and localize/tools are a bit special since they are not part of APF/using APF. For these we could manually bundle d.ts (runtime code already is) -- see #45727.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: packaging Issues related to Angular's creation of npm packages P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

No branches or pull requests

5 participants