Handle single quote escaping in ids

[guybedford]

This ensures that ids containing a single quote are properly escaped when writing in the finalizers to ensure that valid JavaScript is outputted.

For an example of a package with a quote specifier see https://www.npmjs.com/package/@chappa'ai/update-nested-dependencies.

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

[rollup-bot]

Thank you for your contribution! ❤️

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

npm install rollup/rollup#quoteids

or load it into the REPL:
https://rollupjs.org/repl/?circleci=11926

[codecov]

Codecov Report

Merging #3632 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3632      +/-   ##
==========================================
- Coverage   96.54%   96.53%   -0.01%     
==========================================
  Files         182      183       +1     
  Lines        6243     6242       -1     
  Branches     1832     1830       -2     
==========================================
- Hits         6027     6026       -1     
  Misses        107      107              
  Partials      109      109              

Impacted Files	Coverage Δ
src/Chunk.ts	100.00% <100.00%> (ø)
src/utils/escapeId.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 fd57d14...54fb204. Read the comment docs.

[lukastaegert]

Good catch. Question: Instead of patching all finalisers, wouldn't it make more sense to sanitize all ids centrally? Otherwise it is easy to forget places where this is necessary (such as in umd.ts 😉) and it also adds quite a bit of noise. Non-relative external ids could be sanitized here

rollup/src/Chunk.ts

Line 941 in fd57d14

id = dep.renderPath;

while relative external ids and chunk ids could be sanitized here

rollup/src/Chunk.ts

Line 636 in fd57d14

renderedDependency.id = this.getRelativePath(depId, false);

[guybedford]

Yeah that's much nicer - I've integrated it to those code paths.

I also added detection for newlines so that this escaping is now comprehensively spec compliant.

[lukastaegert]

I extended the test to make sure escaping actually works for renormalized external paths and also covers line-breaks. Unfortunately as the test uncovered, neither was true: Renormalized paths were broken because they were escaped twice, which I fixed by moving all escaping logic to the same place. And line-breaks were not escaped as you claimed because preceding \n with a \\ does not give you a \\n, which I fixed by adding a conversion table. Should be working now.

[guybedford]

Thanks so much @lukastaegert, I did not consider the renormalized cases there.

For the line breaks, it is actually valid to have escaped line breaks in JS, see http://rollupjs.org/repl/?version=2.16.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJ2V4dGVyJTVDJTVDJTVDbm5hbCclM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE, but the table works too!