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

Remove jsesc #490

Merged
merged 2 commits into from
Apr 6, 2017
Merged

Remove jsesc #490

merged 2 commits into from
Apr 6, 2017

Conversation

@codecov
Copy link

codecov bot commented Apr 3, 2017

Codecov Report

Merging #490 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #490      +/-   ##
=========================================
- Coverage   82.62%   82.6%   -0.02%     
=========================================
  Files          35      35              
  Lines        2630    2627       -3     
  Branches      928     927       -1     
=========================================
- Hits         2173    2170       -3     
  Misses        279     279              
  Partials      178     178
Impacted Files Coverage Δ
.../babel-plugin-minify-constant-folding/src/index.js 82.53% <ø> (-0.8%) ⬇️

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 b42a9d8...58d3571. Read the comment docs.

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Apr 3, 2017
@goto-bus-stop
Copy link
Contributor

Hmm, the problem in #413/#440 isn't isScriptContext, but all the other things that jsesc does (like escaping backslashes). isScriptContext was the solution to the earlier problem in #382, where </script> could end up in the output inlined in an HTML <script> tag.

A solution entirely in the constant folding plugin would be to manually replace </script><\/script> and <!-- to \x3C!--, which is what the isScriptContext option does

@boopathi
Copy link
Member Author

boopathi commented Apr 3, 2017

Yes. The problem is running jsesc twice. Babel generator already passes all String Literals via jsesc. I'll add a PR to babel to pass isScriptContext to the generator- but probably will be added to the current track - 7.0. And remove jsesc from babili and this option after that.

@goto-bus-stop
Copy link
Contributor

Yes but this would still run jsesc twice, wouldn't it?

@boopathi
Copy link
Member Author

boopathi commented Apr 4, 2017

Will remove jsesc after babel/babel#5581 .

+ Adds option isScriptContext to constant folding plugin
+ Fix #440, fix #413
+ Related #384, #382
@boopathi boopathi added the Tag: Breaking Change Pull Request breaks the API label Apr 4, 2017
@boopathi boopathi changed the title Make jsesc isScriptContext optional Remove jsesc Apr 4, 2017
@boopathi
Copy link
Member Author

boopathi commented Apr 6, 2017

Merging this and reopening #382 . #382 will be fixed via an option in babel.

@boopathi boopathi merged commit 2af84b6 into master Apr 6, 2017
@boopathi boopathi deleted the script-context-0 branch April 6, 2017 18:20
@boopathi boopathi modified the milestone: 1.0 Apr 6, 2017
@mathiasbynens
Copy link
Contributor

mathiasbynens commented Apr 6, 2017

For context, see #440 (comment). This patch is a part of that solution. 👍

@boopathi
Copy link
Member Author

boopathi commented Apr 6, 2017

Yes !! babel/babel#5581 .. That's why I kept #382 open. So we can have an enhancement issue open rather than a bug open - and merged this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: Breaking Change Pull Request breaks the API Tag: Bug Fix Pull Request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary escaping in string concatinaion Invalid string concatenation breaks Lodash _.template method
4 participants