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

Don't allow : in file names. #3972

Merged
merged 2 commits into from Feb 26, 2021
Merged

Conversation

@NfNitLoop
Copy link
Contributor

@NfNitLoop NfNitLoop commented Feb 23, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

File paths with a colon in them can refer to an NTFS "alternate data
stream" which does not operate like a normal file and can break many use
cases. They should be avoided (except for the case of the Windows drive
letter, like "C:").

This reverts some of the test changes made in
87586df which removed the : from
the virtual entry names. This seems to be a convention in some plugins.
The test now tests that those names don't result in invalid Windows file
paths.

I originally uncovered this in my own project, and it's blocking me from properly bundling Rollup'd code (via Snowpack) in my Windows build: NfNitLoop/feoblog#16

File paths with a colon in them can refer to an NTFS "alternate data
stream" which does not operate like a normal file and can break many use
cases. They should be avoided (except for the case of the Windows drive
letter, like "C:").

This reverts some of the test changes made in
87586df which removed the `:` from
the virtual entry names. This seems to be a convention in some plugins.
The test now tests that those names don't result in invalid Windows file
paths.
Copy link
Member

@lukastaegert lukastaegert left a comment

Looks good

@codecov
Copy link

@codecov codecov bot commented Feb 26, 2021

Codecov Report

Merging #3972 (d93d281) into master (9b8c94d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3972   +/-   ##
=======================================
  Coverage   97.21%   97.21%           
=======================================
  Files         191      191           
  Lines        6706     6708    +2     
  Branches     1961     1962    +1     
=======================================
+ Hits         6519     6521    +2     
  Misses         99       99           
  Partials       88       88           
Impacted Files Coverage Δ
src/utils/sanitizeFileName.ts 100.00% <100.00%> (ø)

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 9b8c94d...d93d281. Read the comment docs.

@lukastaegert lukastaegert merged commit 85304f2 into rollup:master Feb 26, 2021
9 checks passed
9 checks passed
@github-actions
14 (Windows)
Details
@github-actions
12 (Windows)
Details
@github-actions
10 (Windows)
Details
ci/circleci: analysis Your tests passed on CircleCI!
Details
ci/circleci: node-v10-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v12-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v14-latest Your tests passed on CircleCI!
Details
@codecov
codecov/patch 100.00% of diff hit (target 97.21%)
Details
@codecov
codecov/project 97.21% (+0.00%) compared to 9b8c94d
Details
This was referenced Mar 6, 2021
This was referenced Mar 16, 2021
guybedford added a commit to guybedford/rollup that referenced this pull request Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants