Revert "Don't allow : in file names. (#3972)"

[guybedford]

This reverts commit 85304f2.

Modules are URLs in Node.js and on the Web. RollupJS outputs relative paths which are relative URLs.

Banning relative URLs that contain : is thus banning valid URL usages, and this breaks support for eg JSPM builds which rely on relative paths like '../npm:pkg@version' to be possible to build.

@NfNitLoop perhaps you could ban : at the readFile level when assuming a path rather? I think there would be more flexibility for RollupJS to keep it open for the internal format to be fully URL compatible (eg building URLs in future as well for eg Deno workflows).

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

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install guybedford/rollup#revert-3972

or load it into the REPL:
https://rollupjs.org/repl/?pr=4050

[codecov]

Codecov Report

Merging #4050 (14c76de) into master (a4a3b8a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 14c76de differs from pull request most recent head f643792. Consider uploading reports for the commit f643792 to get more accurate results

@@            Coverage Diff             @@
##           master    #4050      +/-   ##
==========================================
- Coverage   97.48%   97.48%   -0.01%     
==========================================
  Files         192      192              
  Lines        6788     6786       -2     
  Branches     1996     1995       -1     
==========================================
- Hits         6617     6615       -2     
  Misses         84       84              
  Partials       87       87              

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 a4a3b8a...f643792. Read the comment docs.

[NfNitLoop]

Modules are URLs in Node.js and on the Web. RollupJS outputs relative paths which are relative URLs.

Banning relative URLs that contain : is thus banning valid URL usages, and this breaks support for eg JSPM builds which rely on relative paths like '../npm:pkg@version' to be possible to build.

? and * are also valid in URLs, but these reverted changes still replace them with _. So it seems like the problem isn't "being fully URL compatible" but more specifically "Oops, this change breaks JSPM builds which rely on paths with colons in them."

Unfortunately, I'm not familiar with JSPM, or the "readFile level" you mention, so I'm probably not the best person to make that fix.

I'm curious if Rollup aims to support Windows in the future though.

[NfNitLoop]

Additionally, if we've got contention between rewriting : and not rewriting : that seems worthy of some code comments so that the next person to touch that code block doesn't unknowingly step onto that land mine.

Is it worth adding a unit test explicitly for JSPM build support?

[NfNitLoop]

"Oops, this change breaks JSPM builds which rely on paths with colons in them."

And just to explicitly call this out -- this revert likely won't fix that issue on Windows, if the code relies on writing directory paths that contain :. (But I understand if you want to revert to restore functionality on systems that can have : in file/dir names.)

[lukastaegert]

Maybe instead of reverting and back, we should find a solution that helps everyone? As for myself, I would be super happy to really support arbitrary strings as ids as assuming as little as possible about the target system is part of Rollup's philosophy. Maybe a switch "sanitizeFileNames" which, when activated, sanitizes for maximum compatibility while without the switch, it does not sanitize (but risks failing if you let Rollup just blindly write files to disk). What do you think?

[NfNitLoop]

Definitely would prefer a solution that fixes both cases! 😊

In my experience, two separate functions with different names is less error prone than one with a switch. Something like “sanitizeModuleURL” and “sanitizeFilesystemPath”?

[lukastaegert]

Not sure I understand. First of all, I do not think we have any issue when reading files. Arbitrary ids for modules should be ok here and should always have been ok.
We only have an issue when generating output. And only, if the output is written to disk. So there should only be a single place where we may or may not sanitise the generated chunk names.
So following that, the option should probably be output.sanitizeChunkNames. At this point of course we need to make a decision how much knowledge we want to put in here. I would say the 95% use case would be to make the output "just work" on most systems, and we currently only have an issue on Windows. So we might say true is for compatibility across all operating system file systems.
If someone wants something else, we could also, in an additional improvement, allow a function to be passed here, though I think you could already write custom sanitisers right now by using functions for entryFileNames and chunkFileNames, so not sure we really need this.

[guybedford]

@NfNitLoop I've posted some thoughts on the root cause of your original issue in NfNitLoop/feoblog#16 (comment). Perhaps one fix is to rename \0node-resolve:empty.js in the CommonJS plugin to not use a : and seems a better separation than this sanitization function.

In terms of more generally tackling this sanitization function, it could be interesting to trace back the original reason for the \0 sanitization at all and what use cases this function was originally designed to support for the CommonJS plugin. Surely these internal specifiers should never be external, so we should warn if they are? Or is it an implicit serialization of implict to explicit specifier conversions?

I'd prefer to treat solving the general sanitization problem as separate to this PR, and this PR as a bug fix. But I'm happy to continue discussion here as well.

[guybedford]

It does sound to me though like renaming \0node-resolve:empty.js to \0node-resolve-empty.js so that the chunk name is node-resolve-empty.js if it's a chunk would be the right fix for the root cause.

[guybedford]

(and specifically in the node resolve plugin itself at https://github.com/rollup/plugins/blame/master/packages/node-resolve/src/index.js#L15)

[lukastaegert]

Surely these internal specifiers should never be external

That is unfortunately no longer true thanks to the preserveModules feature. So if someone uses preserveModules, modules for internal use WILL become chunks and Rollup should be able to write them to disk. So at least the \0 sanitation is important for Rollup.

[lukastaegert]

and this PR as a bug fix

At this point, I am not convinced this is a bug fix but rather trading one bug for another, though in different scenarios experienced by different people.

[guybedford]

@lukastaegert would you like to suggest a route forward then?

[guybedford]

IMO sanitization should not be a core feature, but a plugin feature, and externals should be properly sanitized by the appropriate resolver during externalization.

Does anyone disagree with this revert explicitly because it is deemed a necessary feature for sanitization to handle : in core?

[lukastaegert]

I think it is currently necessary to make preserveModules work, because I think there are quite a few plugins, not all of which are under our control, that use :. My proposed fix is to add a switch to disable it but keep it active as default.

[lukastaegert]

Actually looking at your initial reason for posting this, there is one wrong assumption

RollupJS outputs relative paths which are relative URLs

This is not true. Rollup chunks names are path fragments of the form foo/bar.js without leading . or ... As a matter of fact, paths outside the output directory are decidedly not support. URLs also do not make any sense here. Changing this to support URLs would very much question what Rollup output looks like and how it works.

My feeling is you have a very different issue from the one you are fixing here.

[guybedford]

@lukastaegert a switch to disable sanitization could well work. The chunk name sanitization is definitely what is failing for me, since I have : in chunk names. This PR fixes the JSPM build service.

[lukastaegert]

Out of curiosity, what do your chunk names look like?

Another question to think about, if we allow to disable sanitation, I would assume we disable it altogether, i.e. also keep /0 characters etc. Then this switch would mean: I know what I am doing and will perform all necessary sanitation myself.

[guybedford]

See eg https://jspm.dev/npm:uuid@8!cjs. JSPM does an in-memory build for URLs only.

For the sanitization problem, I guess I'm still not clear on what value is provided by \0 in the first place? Wasn't that invalidated the moment preserveModules landed?

[lukastaegert]

Is this PR superseded by #4058?

[guybedford]

Is this PR superseded by #4058?

Currently, yes, per the feedback here. Although I would still prefer this approach of simplifying and removing over adding as a general principle especially since I believe the original reason for the original PR was misguided. But I will not argue this further.

[lukastaegert]

Closing, see above