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: Keep search in resolveReplacementExtensions #1165

Merged
merged 2 commits into from
Nov 27, 2020

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Nov 26, 2020

Related to #1007 (comment)

The search params were lost after making a partial copy to change the file extension.

Even though the dist-raw is largely a copy of Node's implementation, I believe this specific line is custom in ts-node.

@cspotcode Thanks for all the pointers you wrote in the comment!

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #1165 (063adae) into master (a7aa0af) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1165   +/-   ##
=======================================
  Coverage   79.68%   79.68%           
=======================================
  Files           7        7           
  Lines         709      709           
  Branches      157      157           
=======================================
  Hits          565      565           
  Misses         89       89           
  Partials       55       55           
Flag Coverage Δ
node_10 76.20% <ø> (ø)
node_12_15 76.55% <ø> (ø)
node_12_16 76.55% <ø> (ø)
node_13 78.98% <ø> (ø)
node_14 78.98% <ø> (ø)
node_14_13_0 78.13% <ø> (ø)
node_15 78.98% <ø> (ø)
typescript_2_7 78.98% <ø> (ø)
typescript_latest 78.13% <ø> (ø)
typescript_next 78.13% <ø> (ø)
ubuntu 78.84% <ø> (ø)
windows 78.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7aa0af...063adae. Read the comment docs.

@cspotcode
Copy link
Collaborator

Thanks for sending this! It's a holiday for me so it'll be at least a few days before I review this. When I do, I'll double check that the modified line is a ts-node modification. Should be easy with a diffing tool.

Also, ideally we'd have a test for this, too. I think we can add your original reproduction example to our existing ESM tests. Import the same file twice with different params and have it log a message each time.

@frandiox
Copy link
Contributor Author

@cspotcode I've added a test for this. If you revert my first commit, it will fail.
Enjoy your holidays! 🎉

@cspotcode
Copy link
Collaborator

Looks good, thanks again. I'm gonna merge this, then take this as an excuse to update our copy-pasted code to the latest from node 15 in a followup PR. Then I'll publish to npm.

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

Successfully merging this pull request may close these issues.

2 participants