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

[Modification] plugin-typescript - Support new compiler options of TS 4.5+ - nodenext / node12 #1058

Closed
typhonrt opened this issue Nov 30, 2021 · 22 comments

Comments

@typhonrt
Copy link

typhonrt commented Nov 30, 2021

  • Rollup Plugin Name: @rollup/plugin-typescript
  • Rollup Plugin Version: latest

Expected Behavior / Situation

Allow new module and module resolution compiler options of node12 and nodenext to be activated.

Actual Behavior / Situation

Current behavior of compiler option normalization does not take into account new features of TS 4.5+

Modification Proposal

On lines 48-50 of normalize.ts. If one specifies module as nodenext or node12 it is coerced to esnext under the default condition. This will not work for the new functionality exposed in Typescript 4.5+.

Given the peer dependency for @rollup/plugin-typescript being so low the only thing I can think of is adding in case statements with the actual value for ts.ModuleKind.NodeNext and ts.ModuleKind.Node12 for the module compiler option.
nodenext is 199
node12 is 100

Edit: Note that NodeNext / Node12 support was moved from TS 4.5 to 4.6.

@typhonrt
Copy link
Author

I should add that granted I'm aware that full support for nodenext functionality is postponed to likely TS 4.6 now, but this indeed is something to consider for the imminent nodenext / node12 functionality to be released to stable soon. I don't think the peer dependency level for Typescript is going to be raised much for the rollup plugin, so some sort of modification to the case statement that normalizes the module compiler option is necessary.

@frank-dspeed
Copy link
Contributor

related: #1020 (comment)

i am assigning my self to this issue now as i am aware of the current TypeScript Roadmap and i am using Nightly Typescript anyway for building so it is a no brainer to debug the new modes please add any concerns here i will transport that to the TypeScript Repo if needed so that we save overhead.

@stale stale bot added the x⁷ ⋅ stale label Feb 13, 2022
@stale
Copy link

stale bot commented Feb 16, 2022

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@stale stale bot closed this as completed Feb 16, 2022
@typhonrt
Copy link
Author

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping.

Definitely not stale given TS 4.6 is dropping imminently, so this issue is quite pertinent.

@frank-dspeed
Copy link
Contributor

@typhonrt do not worry i am aware of that even if that is closed

@frank-dspeed
Copy link
Contributor

frank-dspeed commented Feb 17, 2022

This is delayed because i am not even sure if typescript introduces a new ModuleKind at present the code base does not reflect that https://github.dev/microsoft/TypeScript look there and do a full text seach for ModuleKind

the node-resolve plugin could be improved and should support maybe a more strict algo but here is nothing to do at this point in time

this is some what related:

@shellscape shellscape reopened this Feb 17, 2022
@stale stale bot removed the x⁷ ⋅ stale label Feb 17, 2022
@typhonrt
Copy link
Author

typhonrt commented Feb 17, 2022

This is delayed because i am not even sure if typescript introduces a new ModuleKind at present the code base does not reflect that https://github.dev/microsoft/TypeScript look there and do a full text seach for ModuleKind

It does indeed... Both in ModuleKind and ModuleResolutionKind. Pulled from the main repo / not branch:
https://github.com/microsoft/TypeScript/blob/main/src/compiler/types.ts#L6210
https://github.com/microsoft/TypeScript/blob/main/src/compiler/types.ts#L6008

This likely will be a blocker for folks trying to use this functionality w/ TS 4.6.

Should probably also make sure es2022 is accessible as applicable.
https://github.com/microsoft/TypeScript/blob/main/src/compiler/types.ts#L6222

@frank-dspeed
Copy link
Contributor

@typhonrt sorry i do not fully get what the problem will be can you please create a minimal example tsconfig and additional files that will produce some wrong result?

@typhonrt
Copy link
Author

typhonrt commented Feb 17, 2022

@typhonrt sorry i do not fully get what the problem will be can you please create a minimal example tsconfig and additional files that will produce some wrong result?

The initial description provided above in the OP is pretty clear what the problem is regarding. As it relates to a real world application, I have a NPM library package that uses sub-module exports. IE the exports section of package.json. The consuming TS project must use Node12 or NodeNext (preferable) to access these submodule exports w/ TS 4.6+ and needs to pass in NodeNext to the rollup plugin.

As mentioned in the OP as long as the peer dependency for TS remains lower than 4.6 adding the actual integer values in for Node12 / NodeNext and while at it ES2020 / ES2022 is the likely solution.

There is a general peer dependency conflict with the beta 4.6 release of TS (for Svelte preprocess and likely the rollup plugin), so I'm waiting for 4.6 to officially drop in ~5 days to continue testing.

Since my project is open source I'll provide a link to the demo repo consuming the package with sub-module exports after 4.6 drops. The changes specified will make it work.

@frank-dspeed
Copy link
Contributor

@typhonrt i looked more deep into it and i guess the same stuff applys for existing big typescript projects they will need to use indipendent tsconfig.json files as references with individual settings so composit projects will solve that.

i expirenced that in bigger ts projects a single resolve algo will never fit all module situations while "node" solves the most situations

i suggest to create a extra composit project that you call dependencie consumption or something like that and reference the tsconfig.json inside that

then you switch there the resolve algo to "node" then in your main dev project you will consume the composit project does that make sense for you?

@typhonrt
Copy link
Author

@frank-dspeed Sounds like a whole lot more ceremony to be honest and TS doesn't need anymore of that! I'll be forking this plugin and making the changes and submit a PR as the fixes are trivial. Might be a couple of weeks as I'm busy with other dev presently, but this is "on the list".

@frank-dspeed
Copy link
Contributor

frank-dspeed commented Feb 27, 2022

do not get me wrong but the main problem i am facing is the translation of concepts Typescript works with diffrent module types (moduleKind) and rollup works with ESM only. While we tend to transpil into ESM before doing rollup.

So i am not sure if it makes sense to not treat them all as ESM as for rollup only ESM Exists

your use case sounds more like a typescript packaging feature that is also great stuff and WiP. You do not need a fork or anything a simple repo that shows a error with typescript nightly would be more then enough.

@typhonrt
Copy link
Author

Yeah.. I'll post an update when I can get to this. My use case is that I'm delivering a library / development package that uses sub-module exports that includes types in each sub-module export entry. This is all newly supported TS 4.6+ functionality and I really can't require the end developer consuming my package to require a special tsconfig as I'm sure there are other complications downstream let alone to passing on the annoyance to additional developers, etc.

It's going to be anywhere from 2-4 weeks for me to take a look likely as a complete TS validation of my package is on the tail end of the official release TODO list.

@stale stale bot added the x⁷ ⋅ stale label Apr 29, 2022
@stale
Copy link

stale bot commented Apr 30, 2022

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@stale stale bot closed this as completed Apr 30, 2022
@typhonrt
Copy link
Author

typhonrt commented Apr 30, 2022

Heh; not stale. I've been delayed on taking a look, but the TS 4.7 final release is just around the corner and this finally brings full support for node12 / nodenext. Most likely working on this in May.

@typhonrt
Copy link
Author

I'm still delaying any TS support for my library projects for a bit longer, but just want to update things on the state of TS 4.7+ as there is no longer a node12 and this is now node16.

So the proposed fix for the Rollup TS plugin while the peer dependency remains at >= 3.7.0 for TS is to add in specific case statements with the numerical values.

This makes the switch statement from normalize.ts:

    switch (compilerOptions.module) {
        case ts.ModuleKind.ES2015:
        case ts.ModuleKind.ES2020:          // Note ES2020 is used in other parts of the plugin and is a TS 3.8 level addition.
        case ts.ModuleKind.ESNext:
        case ts.ModuleKind.CommonJS:

        // Necessary to add raw values until TS peer dependency is raised significantly.
        case 7: // ES2022
        case 100: // node16
        case 199: // nodenext
            // OK module type

Also change in preflight.ts:

const validModules = [ModuleKind.ES2015, ModuleKind.ES2020, ModuleKind.ESNext, undef, 7, 100, 199];

A note that ES2020 is used which is TS 3.8.x level feature above, but also need to add the 7, 100, 199 for ES2022, Node16, NodeNext.

I suggest bumping the peer dependency of TS to 3.8.0 as that actually has ES2020 as a module config option and include the above numerical references to the more modern TS 4.5+ config options until the peer dependency for this plugin is bumped TS >= 4.7.0

@shellscape
Copy link
Collaborator

Since 4.7 has been out a bit now, I'm in favor of bumping the dependency to match.

@shellscape shellscape reopened this Aug 13, 2022
@Mister-Hope
Copy link

Mister-Hope commented Aug 25, 2022

I am not sure, is this issue being fixed or not?

I try to read rollup and this plugin source code for half an hour, but I was too unfamilar with rollup systerm.

My problem is that I switched to moduleResolution: NodeNext days ago, and I decide to make a remapping in exports filed:

{
  "name": "vuepress-shared",
  "type": "module",
  "exports": {
    ".": "./lib/node/index.js",
    // this line I make <pkg>/client pointing to <pkg>/lib/client/index.js
    "./client": "./lib/client/index.js",
    "./lib/client": "./lib/client/index.js",
    "./lib/client/*": "./lib/client/*",
    "./noopModule": "./lib/client/noopModule.js",
    "./package.json": "./package.json"
  },
}

While importing vuepress-shared/client in other packages in the same monorepo, tsc works pretty well, while rollup gives:

[!] (plugin typescript) Error: @rollup/plugin-typescript TS2307: Cannot find module 'vuepress-shared/client' or its corresponding type declarations.
src/client/composables/useBlogCategory.ts (5:42)

5 import { resolveRouteWithRedirect } from "vuepress-shared/client";

So it seems that rollup will actually let ts find vuepress-shared/client, instead of vuepress-shared/lib/client

I am sure that rollup is actually reading moduleResolution: NodeNext by logging parsedOptions

{
  options: {
    allowSyntheticDefaultImports: true,
    allowUnreachableCode: false,
    allowUnusedLabels: false,
    exactOptionalPropertyTypes: true,
    noFallthroughCasesInSwitch: true,
    strict: true,
    noImplicitOverride: true,
    noImplicitReturns: true,
    noPropertyAccessFromIndexSignature: true,
    noUncheckedIndexedAccess: false,
    noUnusedLocals: true,
    noUnusedParameters: true,
    baseUrl: '/home/mister-hope/projects/vuepress-theme-hope',
    module: 99,
    moduleResolution: 99,
    resolveJsonModule: true,
    downlevelIteration: false,
    importHelpers: true,
    importsNotUsedAsValues: 0,
    newLine: 1,
    noEmitOnError: true,
    removeComments: false,
    inlineSources: false,
    inlineSourceMap: false,
    sourceMap: true,
    allowJs: false,
    checkJs: false,
    esModuleInterop: true,
    forceConsistentCasingInFileNames: true,
    lib: [ 'lib.dom.d.ts', 'lib.esnext.d.ts' ],
    target: 7,
    diagnostics: false,
    listEmittedFiles: false,
    skipLibCheck: true,
    pretty: true,
    rootDir: '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src',
    configFilePath: '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/tsconfig.json',
    noEmitHelpers: true,
    noEmit: false,
    emitDeclarationOnly: false,
    noResolve: false
  },
  watchOptions: undefined,
  fileNames: [
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shims-category.d.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shims-type.d.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/client/index.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/client/composables/index.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/client/composables/useBlogCategory.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/client/composables/useBlogType.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/node/category.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/node/index.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/node/plugin.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/node/type.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/node/utils.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shared/frontmatter.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shared/index.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shared/internal.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shared/options.ts',
    '/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src/shared/types.ts'
  ],
  projectReferences: undefined,
  typeAcquisition: { enable: false, include: [], exclude: [] },
  raw: {
    extends: '../../tsconfig.base.json',
    compilerOptions: {
      module: 'ESNext',
      skipLibCheck: true,
      rootDir: './src',
      moduleResolution: 'NodeNext'
    },
    include: [ './src' ],
    compileOnSave: false
  },
  errors: [],
  wildcardDirectories: {
    "/home/mister-hope/projects/vuepress-theme-hope/packages/blog2/src": 1
  },
  compileOnSave: false,
  autoSetSourceMap: false
}

@frank-dspeed
Copy link
Contributor

@Mister-Hope at last i did not invest into fixing it at present because i am undergoing some general iterations about module systems in bigger groups if you want my 5 cent on this

You should not use the new resolve algos stay with node and create composit project folders for the once where you need node next also bundel them indipendent

@typhonrt
Copy link
Author

typhonrt commented Aug 25, 2022

You should not use the new resolve algos stay with node and create composit project folders for the once where you need node next also bundel them indipendent

This is not the solution though; supporting the most recent module resolution capabilities of TS is necessary to support sub-package exports. I have clearly outlined the code changes above and if the peer dependency is going to stay at TS 3.7 you just have to insert the new / valid module values. If you need a PR with the fix I've outlined above which 100% solves the problem let me know. It'll take 5 minutes to make the PR and submit it. Just takes the will of the maintainers to recognize it as the solution. :)

I know we're all busy.. But this is not going away as a current pain point for those using modern TS features.. ;P

Can you comment why what I have proposed is not acceptable?

@typhonrt
Copy link
Author

typhonrt commented Aug 25, 2022

Looks like there is an independent PR for this issue. #1194
This PR takes the raising the peer dependency to 4.7 approach and makes the fixes and a bit more it seems. I'll close this issue when that PR goes through. :D

@frank-dspeed
Copy link
Contributor

frank-dspeed commented Aug 25, 2022

@NotWoods NotWoods closed this as completed Sep 7, 2022
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

No branches or pull requests

5 participants