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

Fix running bin script for es modules with node resolution #44345

Closed
wants to merge 3 commits into from

Conversation

tsapko3628
Copy link

Fixes Issue #44342

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 31, 2021
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@ghost
Copy link

ghost commented May 31, 2021

CLA assistant check
All CLA requirements met.

@DanielRosenwasser
Copy link
Member

Technically a breaking change if we have people who relied on typescript/bin/tsc as being extensionless - but it's unclear who relied on that.

@tsapko3628
Copy link
Author

Fixed breaking changes. Now it works correctly for

  • yarn tsc
  • yarn run tsc
  • yarn exec tsc
  • npx tsc
  • node_modules/.bin/tsc
  • running tsc in package.json scripts

Also add old tsserver for IDEs who use node_modules/.bin/tsserver

@tsapko3628
Copy link
Author

I fixed it, but then I realized what it wasn't breaking changes. When you install typescript package, package manager will create symlink like this node_modules/.bin/tsc -> node_modules/typescript/bin/tsc.js. So, it wasn't backing changes. I can remove bin/tsc and bin/tsserver if you agree with me

@tsapko3628
Copy link
Author

but it's unclear who relied on that so you know, a lot of people rely on it. I saw node node_modules/.bin/tsc in many projects scripts. Some people do that for adding nodejs cli options, but they also can do it via NODE_OPTIONS env.

@tsapko3628
Copy link
Author

Could somebody review this pr? @rbuckton @RyanCavanaugh @weswigham.
It will take less than 5 minutes.

@weswigham
Copy link
Member

You've found a bug in node's --experimental-specifier-resolution=node flag, not in TS. (It's not supposed to affect existing cjs code at all, after all!) You should probably report it in the node repo.

@tsapko3628
Copy link
Author

Might be, but it happening because of file without extensions. You can solve it just by adding extension without any braking changes at all.

@tsapko3628
Copy link
Author

tsapko3628 commented Jun 7, 2021

nodejs/node#35518
So, because of this, it's a bug for es modules
@weswigham

@sandersn sandersn added this to Not started in PR Backlog Jun 8, 2021
@sandersn sandersn moved this from Not started to Waiting on author in PR Backlog Jun 8, 2021
@sandersn
Copy link
Member

@weswigham is it worth taking this fix or should we wait for a fix from node? I'm not clear on whether that will address the original bug.

@tsapko3628
Copy link
Author

So, what about this? Nodejs major release doesn't fix problem and it maybe won't fix in next release too.
No braking changes, work with flag and without it. Looks like it'll be good for 'flag' users and won't make troubles for other users

@weswigham
Copy link
Member

Yeah, we're not changing anything for this. Anything nonfunctional is a bug in an experimental feature in node, which we'll not bend over backwards to support ♥ - our bin file is a cjs file in a cjs package; any configuration of node that breaks that is a problem in node itself, of that I'm confident.

@weswigham weswigham closed this Mar 4, 2022
PR Backlog automation moved this from Waiting on author to Done Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Use explicit extensions in TypeScript's package.json for Node ES resolution compatibility
5 participants