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

support newer moduleResolution kinds, update build to TS 5.x #453

Merged
merged 10 commits into from Jul 17, 2023

Conversation

ezolenko
Copy link
Owner

@ezolenko ezolenko commented Jun 20, 2023

Summary

Fixes #437

Details

  • keeping node* moduleResolution option as is ("node", "node16", "nodenext", "bundler")
  • overriding "classic" and empty to Node10 (old behavior was overriding everything to Node10)

@ezolenko ezolenko marked this pull request as ready for review June 23, 2023 16:08
@agilgur5 agilgur5 changed the title Bug/437 2 add module: 'ES2022', update moduleResolution default to node16 Jun 30, 2023
@agilgur5 agilgur5 changed the title add module: 'ES2022', update moduleResolution default to node16 add module: 'ES2022', update moduleResolution default to node16, update to to TS 5.x Jun 30, 2023
@agilgur5 agilgur5 changed the title add module: 'ES2022', update moduleResolution default to node16, update to to TS 5.x add module: 'ES2022', update moduleResolution default to node16, update build to TS 5.x Jun 30, 2023
@agilgur5 agilgur5 changed the title add module: 'ES2022', update moduleResolution default to node16, update build to TS 5.x update moduleResolution default to node16, update build to TS 5.x Jun 30, 2023
@agilgur5 agilgur5 changed the title update moduleResolution default to node16, update build to TS 5.x update moduleResolution overrides, update build to TS 5.x Jun 30, 2023
@agilgur5 agilgur5 changed the title update moduleResolution overrides, update build to TS 5.x support moduleResolution overrides, update build to TS 5.x Jun 30, 2023
@agilgur5 agilgur5 changed the title support moduleResolution overrides, update build to TS 5.x support newer moduleResolution kinds, update build to TS 5.x Jun 30, 2023
Comment on lines 34 to 36
case tsModule.ModuleResolutionKind.Bundler:
default:
overrides.moduleResolution = tsModule.ModuleResolutionKind.Node10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm gonna need to review this in a lot more detail (need to catch up on TS changes since 5.x hit GA and refresh myself on this codebase), but I mentioned mjs support and bundler in #434 (comment).

bundler actually seemed the most appropriate at the time (since Rollup is a bundler) for Node16+, particularly due to the file extension issues. That's actually why I didn't implement support for mjs / cjs earlier because the file extension requirement could break many things (that and no user requested it)

Copy link
Owner Author

@ezolenko ezolenko Jun 30, 2023

Choose a reason for hiding this comment

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

Yeah, this basically lowest effort to let users use more moduleResolution values without figuring out exactly what the differences are.

Copy link
Collaborator

@agilgur5 agilgur5 Jul 11, 2023

Choose a reason for hiding this comment

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

So I think bundler should not be overwritten either. With that change and README modifications about this, I think we could approve and merge this in.

Without tests though, we won't be able to confirm some of the behavior of node16 / nodeNext (particularly around file extensions) -- it is possible we may need to override those two to bundler as well

ezolenko and others added 2 commits July 11, 2023 21:34
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

mostly LGTM now, just left a tiny comment style comment. we'll ofc iterate on the docs as we test more and get feedback

README.md Outdated Show resolved Hide resolved
dist/rollup-plugin-typescript2.cjs.js Outdated Show resolved Hide resolved
ezolenko and others added 2 commits July 17, 2023 09:03
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
@ezolenko ezolenko merged commit ce2038d into master Jul 17, 2023
6 checks passed
@ezolenko ezolenko deleted the bug/437_2 branch July 17, 2023 15:22
@agilgur5 agilgur5 added the kind: feature New feature or request label Jul 18, 2023
@agilgur5 agilgur5 added version: minor Increment the minor version when merged topic: TS version Related to a change in a TS version solution: needs test This issue requires creating a test to assuredly close out labels Jul 18, 2023
@juliaschiller150
Copy link

Hey team! Thanks for this. Wondering when it might get released / published to npm?

@ezolenko
Copy link
Owner Author

@tcschiller could you test to see if this PR works for your use-case?

@juliaschiller150
Copy link

juliaschiller150 commented Sep 26, 2023

@tcschiller could you test to see if this PR works for your use-case?

Pulled down latest from the master branch and ran the following:

npm ci
npm run build
npm run build-self
npm run build-self
npm pack

Then I tried my own repo, first, without that tarball. Built my library and successfully reproduced the problem at hand - it can't resolve imports with relative paths (e.g., import { myExport } from '@scope/package-name/submodule').

Then I ran in my own repo:

npm install ../rollup-plugin-typescript2/rollup-plugin-typescript2-0.35.1.tgz

And immediately retried the build which, up to this point, was failing.

Could no longer reproduce the problem; it seems the MR discussed here fixes everything (because my TypeScript config has the bundler value for moduleResolution).

Thanks for the hard work, @ezolenko . Hope the above proves helpful!

@agilgur5
Copy link
Collaborator

Thanks for testing this out @tcschiller!

For reference, you can reference a GH repo directly in NPM (and any git repo as well) and use a SHA. This commit in particular made a change to the dist folder in the repo, so it already has the compiled version. Very useful for exactly this kind of scenario 🙂

@agilgur5
Copy link
Collaborator

I was also curious about node16 / nodeNext above, specifically whether we should force bundler on those b/c of the file extension requirements.

I approved a few months ago since this is still better than the previous state, but that was the one remaining question on this PR

@ezolenko
Copy link
Owner Author

Well, shouldn't break anything. I'll release this sometime tomorrow.

@ezolenko
Copy link
Owner Author

ezolenko commented Sep 27, 2023

In 0.36.0 now

@PetarKirov
Copy link

@ezolenko with 0.36.0 I'm getting the following error:

[!] (plugin rpt2) Error: Incompatible tsconfig option. Module resolves to 'Node16'. This is incompatible with Rollup, please use 'module: "ES2015"', 'module: "ES2020"', 'module: "ES2022"', or 'module: "ESNext"'.

With the following tsconfig settings:

{
  "compilerOptions": {
    "module": "Node16",
    "moduleResolution": "Node16",
    // ...
  }
}

And the following Rollup plugin config settings:

  const fileExtensions = ['.mjs', '.cjs', '.js', '.ts', '.json'];

  const plugins = [
    json(),
    commonjs(),

    peerDepsExternal({
      packageJsonPath: path.join(dir, 'package.json'),
    }),
    nodeResolve({
      extensions: fileExtensions,
      preferBuiltins: true,
      exportConditions: ['node'],
      mainFields: ['module', 'main'], // This instructs Rollup to prioritize ESM over CJS when resolving modules
      modulesOnly: true,
    }),
    typescript({
      check: true,
      tsconfig: path.join(dir, 'tsconfig.lib.json'),
      useTsconfigDeclarationDir: true,
    }),
    del({ targets: `${dir}/dist/*` }),
  ];

I'm using the package following versions:

  • rollup: 3.29.4
  • @rollup/plugin-node-resolve: 15.2.1
  • rollup-plugin-typescript2: 0.36.0

As far as I can see, this comes from these lines:

https://github.com/ezolenko/rollup-plugin-typescript2/blob/0.36.0/src/parse-tsconfig.ts#L46-L48

@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 28, 2023

@PetarKirov this PR only changed the moduleResolution options, not the module options.

There are potentially additional changes needed for the module options, but for the most part, we'd likely just be overriding it to ESNext or something. As Rollup requires ESM throughout, CJS output would be incompatible (TS can output CJS for module node16 and nodenext). So the error is more or less still accurate.

If you need node16 for other tooling, you can use tsconfigOverride to set esnext or es2020 for rpt2 specifically.

@raphael-theriault-swi
Copy link

This doesn't work with either node16 or nodenext since they require the module field to match

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request solution: needs test This issue requires creating a test to assuredly close out topic: TS version Related to a change in a TS version version: minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support moduleResolution: 'node16'+ for package.json exports
5 participants