Skip to content

Fix: Keep search in resolveReplacementExtensions - #1165

Merged
cspotcode merged 2 commits into
TypeStrong:masterfrom
frandiox:patch-1
Nov 27, 2020
Merged

Fix: Keep search in resolveReplacementExtensions#1165
cspotcode merged 2 commits into
TypeStrong:masterfrom
frandiox:patch-1

Conversation

@frandiox

@frandioxfrandiox commented Nov 26, 2020

Copy link
Copy Markdown
Contributor

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

codecovBot commented Nov 26, 2020

Copy link
Copy Markdown

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 
FlagCoverage Δ
node_1076.20% <ø> (ø)
node_12_1576.55% <ø> (ø)
node_12_1676.55% <ø> (ø)
node_1378.98% <ø> (ø)
node_1478.98% <ø> (ø)
node_14_13_078.13% <ø> (ø)
node_1578.98% <ø> (ø)
typescript_2_778.98% <ø> (ø)
typescript_latest78.13% <ø> (ø)
typescript_next78.13% <ø> (ø)
ubuntu78.84% <ø> (ø)
windows78.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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@cspotcode

Copy link
Copy Markdown
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 freeto 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

@frandiox@cspotcode