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: toExports extension strip #291

Merged
merged 6 commits into from
Nov 9, 2023
Merged

fix: toExports extension strip #291

merged 6 commits into from
Nov 9, 2023

Conversation

GerryWilko
Copy link
Contributor

@GerryWilko GerryWilko commented Oct 28, 2023

πŸ”— Linked issue

#290

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This resolves issue #290 by allowing package names with . in the name to be left unchanged.

This change alters the regex used to match the extension to ensure that it only changes the last segment of the path and when no preceeding / or ./ is present in the segment then do not alter the path.

I include some outputs of the testing I have done with this Regex in my browser console.

image

I do not believe this will be a breaking change as the only change in behaviour here, I believe, is to ensure that only the extension is dropped other segments of the path are unchanged.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@GerryWilko
Copy link
Contributor Author

GerryWilko commented Oct 28, 2023

There are two tests that failed but I believe they may be incorrect tests.

https://github.com/unjs/unimport/blob/7df98d0a6a7b53437514eb078c295dfc5ec86087/test/to-export.test.ts#L66C32-L66C33

These tests actually should fail as the from may include a package name such as chart.js which will have the .js stripped. The first segment of the path without a preceeding / or ./ should not be altered as this would be the package name.

I have altered these tests and added some new tests for extensions stripping checks.

@manniL manniL requested review from pi0 and danielroe November 1, 2023 09:27
@GerryWilko
Copy link
Contributor Author

@manniL sorry that was silly of me 😞. Fixed the double quote usage

@manniL
Copy link
Member

manniL commented Nov 1, 2023

No biggie at all. AutoFix CI would've resolved that too I think πŸ˜‹

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #291 (22648ff) into main (7df98d0) will increase coverage by 0.01%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 22648ff differs from pull request most recent head 3c77a72. Consider uploading reports for the commit 3c77a72 to get more accurate results

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   97.41%   97.42%   +0.01%     
==========================================
  Files          57       57              
  Lines        1507     1513       +6     
  Branches      308      311       +3     
==========================================
+ Hits         1468     1474       +6     
  Misses         39       39              
Files Coverage Ξ”
src/utils.ts 97.79% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

@danielroe
Copy link
Member

What about checking for existence of file? otherwise, even with the improvements here, it might be impossible to distinguish /some/chart.js from /some/chart.js/index.js or /some/chart.js.js....

@antfu
Copy link
Member

antfu commented Nov 2, 2023

Checking for the FS could be inefficient and make unimport less portable. Maybe we could check if the import from is a path like (start with '.', '/', '\', '[\w+]://') and only strip on that?

@GerryWilko
Copy link
Contributor Author

I suppose also checking for file existing could break workflows like using Prisma client where the client is only present after generate.

@antfu this looks like a sensible approach to me. Regexes are not my forte πŸ˜ƒ

@GerryWilko
Copy link
Contributor Author

GerryWilko commented Nov 2, 2023

What about checking for existence of file? otherwise, even with the improvements here, it might be impossible to distinguish /some/chart.js from /some/chart.js/index.js or /some/chart.js.js....

@danielroe I think this regex caters for this? Apologies if I am misunderstanding but I believe this regex only strips the last segments extension. I added to the unit tests some examples of this.

@danielroe
Copy link
Member

Checking for the FS could be inefficient and make unimport less portable. Maybe we could check if the import from is a path like (start with '.', '/', '', '[\w+]://') and only strip on that?

sounds like a good idea to me πŸ‘

edge cases can be avoided by making sure to fully resolve absolute/relative paths.

@antfu antfu merged commit cf28aa3 into unjs:main Nov 9, 2023
3 checks passed
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.

4 participants