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

Cannot bundle a TS file, which imports JS file from a package using NodeJS conditional exports #911

Closed
bhovhannes opened this issue Jun 23, 2021 · 6 comments · Fixed by #921

Comments

@bhovhannes
Copy link
Contributor

bhovhannes commented Jun 23, 2021

Hi,

Issue description

Steps to reproduce

  1. git clone the https://github.com/bhovhannes/rollup-plugin-node-resolve-issue repo
  2. run npm ci
  3. run npm run build

Details

The reproduction link written above is a small Rollup project. Entrypoint - index.ts, is in TypeScript. TypeScript does not really matter here, the only thing which matters is the extension of the file - .ts.

Inside an entrypoint file we import a utility function using import sum from '@abcdef/lib-utils/dist/sum.js' syntax.

The package @abcdef/lib-utils has an exports field in its package.json containing:

"exports": {
    "./dist/*": {
        "import": "./dist_esm/*",
        "require": "./dist/*"
    }
}

That makes possible to let Rollup consume sum.js from dist_esm folder and if the tool does not support ESM (looking on you Jest) it will use sum.js from dist folder.

Lines https://github.com/rollup/plugins/blame/master/packages/node-resolve/src/index.js#L148-L155 in @rollup/plugin-node-resolve code check for the case when there is a .js import inside a .ts file and change extension to .ts. I'll be grateful for any explanation why this is needed.
After that, plugin resolves the package, taking exports field into account and ends up with node_modules/@abcdef/lib-utils/dist_esm/sum.ts url, which obviously does not exist.

Expected Behavior

Build should complete without errors.

Actual Behavior

Build errors out, trying to bundle non-existent TypeScript file. The error:

Error: Could not load /Users/hovhannesbabayan/Projects/github/bhovhannes/
rollup-plugin-node-resolve-issue/node_modules/@abcdef/lib-utils/dist_esm/sum.ts (imported by input.ts): 

ENOENT: no such file or directory, open '/Users/hovhannesbabayan/Projects/github/bhovhannes/
rollup-plugin-node-resolve-issue/node_modules/@abcdef/lib-utils/dist_esm/sum.ts'

Additional Information

Removing lines https://github.com/rollup/plugins/blame/master/packages/node-resolve/src/index.js#L148-L155 fixes the issue.

Or, perhaps we can check if the resolved file indeed exists in https://github.com/rollup/plugins/blob/master/packages/node-resolve/src/package/resolvePackageTarget.js#L13 ?

I'll be happy to submit a pull request, just I am not sure in which direction to proceed.

Thanks!

@bhovhannes
Copy link
Contributor Author

@tjenkinson and/or @guybedford sorry for tagging you directly, but you both worked on last big change in the part of code which is responsible for resolving files.
Can you please provide any directions about how to proceed there?

@tjenkinson
Copy link
Member

Hi @bhovhannes could you try removing the lines you mentioned and see if any tests fail? That might help explain the reason for it. On the surface I’m also a bit confused why it gets rewritten there.

@bhovhannes
Copy link
Contributor Author

bhovhannes commented Jun 30, 2021

@tjenkinson removing lines failed one test, which was introduced by PR #480, which fixed issue #479.

Everywhere it is said that TS does support importing other .ts files via .js extension which is not something I understand. Why would people ever want to write import {something} from "./file.js" when there is no "file.js" but only "file.ts"?
Maybe I am missing something here.

@bhovhannes
Copy link
Contributor Author

bhovhannes commented Jun 30, 2021

well, I found that in TypeScript docs: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#module-identifiers-allow-for-js-extension

And the support for that has been added discussed in webpack's ts-loader as well.

Which means the lines cannot be removed I guess.

@tjenkinson
Copy link
Member

Thanks. After looking at the issue and pr I don’t get why it was added. From the screenshots it looks like it might have been to workaround a linter error for extensionless import(?)

If typescript really tries to switch an explicit .js extension for a .ts one then I’m baffled.

Need to spend more time looking into this.

@tjenkinson
Copy link
Member

Opened a PR to fix here: #921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants