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

moduleResolution: node12 support #45884

Merged
merged 31 commits into from
Sep 24, 2021

Conversation

weswigham
Copy link
Member

This PR is mostly just to pack up everything together for reviews over at weswigham#68 (if you're pinged as a reviewer here, go there to just review this most recent stage of development). It's also a viable PR to squash and merge to get everything merged all at once, once reviews and CI are in~

@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 15, 2021

Heya @weswigham, I've started to run the tarball bundle task on this PR at 859abb6. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 15, 2021

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/110514/artifacts?artifactName=tgz&fileId=A65D5A5264B9F1A3596294CAC5C757AD0A232C44FB117FB455A4D75D5A2F703C02&fileName=/typescript-4.5.0-insiders.20210915.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@weswigham weswigham merged commit 586b0d5 into microsoft:main Sep 24, 2021
@frank-dspeed
Copy link

frank-dspeed commented Oct 14, 2021

@weswigham i did test this beta get

Error: @rollup/plugin-typescript TS5070: Option '--resolveJsonModule' cannot be specified without 'node' module resolution strategy.
Error: @rollup/plugin-typescript TS5070: Option '--resolveJsonModule' cannot be specified without 'node' module resolution strategy.

so support for moduleResolution node12 and nodenext is needed in the array of supported modes for resolveJsonModule

#46362

@JounQin
Copy link

JounQin commented Oct 18, 2021

I just tried the typescript@next and moduleResolution: 'node12' with @angular/compiler@v13, but tsc -b failed to build:

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.

@angular/compiler@v13 is ESM only, ParsedTemplate is typing exported from its types entry.

See https://unpkg.com/browse/@angular/[email protected]/package.json

synckit is both commonjs and ESM compatible.

See https://unpkg.com/browse/[email protected]/package.json

I have no idea how can it be fixed on my side.


Test source codes:

// worker.ts
import type { ParsedTemplate } from '@angular/compiler'
import { runAsWorker } from 'synckit'

runAsWorker<ParsedTemplate>(async (code: string, filePath: string) => {
  const { parseTemplate } = await import('@angular/compiler')
  return parseTemplate(code, filePath, {
    preserveWhitespaces: true,
    preserveLineEndings: true,
    collectCommentNodes: true,
  })
})

for (const name of names) {
let resolution = resolutionsInFile.get(name);
const mode = containingSourceFile ? getModeForResolutionAtIndex(containingSourceFile, i) : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot assume that module name is referenced at index i in containing source file because we could be calling you with only few module names from the source file. https://github.com/microsoft/TypeScript/blob/main/src/compiler/program.ts#L1571 shows that case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants