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

Allow to update require path with Node.js #53964

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

g3offrey
Copy link
Contributor

@g3offrey g3offrey commented Jul 10, 2018

Resolves : microsoft/TypeScript#25493
Resolves : #53681

Fix a regression appearing with : 1ee1759
Thanks for your help @mjbvz and @andy-ms 👍

If something should be changed in this PR don't hesitate to notice it to me. 😄

@ghost
Copy link

ghost commented Jul 10, 2018

The question of whether an update is needed can't be answered by looking at just one file. There might be a reference to even a non-module by a /// <reference> or through a tsconfig.json. So I think it would be better to remove this test entirely and unconditionally ask tsserver.

@mjbvz
Copy link
Collaborator

mjbvz commented Jul 10, 2018

Yes this was a workaround to try to prevent microsoft/TypeScript#24914 from happening. I'll merge this but also add a version check so that we only do this on TS 2.9 and not on 3.0

@mjbvz mjbvz added this to the July 2018 milestone Jul 10, 2018
@mjbvz mjbvz merged commit 2ea8428 into microsoft:master Jul 10, 2018
@g3offrey
Copy link
Contributor Author

g3offrey commented Jul 11, 2018

It seems a better fix is coming ! Can't wait for it to land on my editor 👍

Thanks @mjbvz and @andy-ms for helping me with this issue and for merging my PR.
That's very nice of you 😄

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants