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
Handle single quote escaping in ids #3632
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via
or load it into the REPL: |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Line 941 in fd57d14
id = dep.renderPath; |
Line 636 in fd57d14
renderedDependency.id = this.getRelativePath(depId, false); |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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! |
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:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description