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

[Bug] Removal of quotes for property names with special unicode still broken #627

Closed
patrick-steele-idem opened this issue Jul 11, 2017 · 12 comments
Labels
bug Confirmed bug

Comments

@patrick-steele-idem
Copy link

patrick-steele-idem commented Jul 11, 2017

Despite PR #543 - Fix removal of quotes from property names, there is still a problem with how quotes are removed from property names that can be noticed when trying to load the minified he library in the latest Safari.

To clarify, given the following input file:

module.exports = {
    '\uD835\uDCB6': 'ascr'
};

The babili output is the following:

module.exports={𝒶:'ascr'}

If you try to load that in Safari you get the following error:

SyntaxError: Invalid character '\u55349'

Thoughts?

@patrick-steele-idem
Copy link
Author

/cc @boopathi @mathiasbynens

@patrick-steele-idem
Copy link
Author

Also, please see my earlier comment:

I did some additional investigation and found that the babel-plugin-minify-constant-folding plugin will also introduce Identifier nodes for object keys even if the key name contains special unicode characters because t.isValidateIdentifier() doesn't restrict to safe identifiers (safe for currently the latest Safari v10.0.3, that is). The babel-plugin-transform-property-literals plugin only transforms string literal keys (not identifier keys). Therefore, even with this patch, he does not get minified in a way that can be loaded in the latest Safari.

It looks like both babel-plugin-minify-constant-folding and babel-plugin-transform-property-literals are causing problems.

@timsuchanek
Copy link

This is indeed a problem, our build is now working in Firefox with the changes of #543 but still doesn't work with Safari. So this is the last issue holding us back from migrating completely to babili

@damianobarbati
Copy link

Using babili I get the following!

SyntaxError: Invalid character '\u2160'

Around here:

Copyright (c) 2016 Jed Watson.
  Licensed under the MIT License (MIT), see
  http://jedwatson.github.io/classnames
*/ <== console brings me here!

@mathiasbynens
Copy link
Contributor

@damianobarbati Can you link to a test case that reproduces the issue? That comment doesn’t include U+2160.

@damianobarbati
Copy link

Unfortunately I can't 😞
And I don't know why the console brings me over there.
If I manage to reproduce this in a repro I'll be sharing that here for sure, thanks @mathiasbynens

@spudly
Copy link

spudly commented Oct 11, 2017

I just ran into the same problem as @patrick-steele-idem.

Worked around the issue by adding the following options to the minify command: --no-evaluate --no-propertyLiterals

This disables the two plugins that are breaking the output code (babel-plugin-minify-constant-folding and babel-plugin-transform-property-literals).

@JayCarney
Copy link

For anybody running into this using babel-minify-webpack-plugin worked around this using the advice from @spudly using:

const MinifyPlugin = require('babel-minify-webpack-plugin');

// webpack settings
plugins: [
  new MinifyPlugin({
    evaluate: false, 
    propertyLiterals: false
  })
]

@boopathi boopathi added the bug Confirmed bug label Oct 26, 2017
@Awk34
Copy link

Awk34 commented Nov 9, 2017

I'm getting the same issue with https://github.com/mozilla/pdf.js . The first place it starts failing is for \u2160, which is Roman Numeral One (Ⅰ). The same workaround to disable property minification fixed it for me. Here's the line in their source code: https://github.com/mozilla/pdf.js/blob/v1.9.426/src/core/unicode.js#L335

@jogibear9988
Copy link
Contributor

seems like the conversation of the literal is already done in babel.

bildschirmfoto 2017-11-18 um 09 16 15

@jogibear9988
Copy link
Contributor

In "babel-plugin-minify-constant-folding" it can be solved by inserting

	if (t.isObjectExpression(node)) {
	  for (var p of node.properties) {
		 if ((p.key.extra.raw[0] == '\'' && p.key.extra.raw != '\'' + p.key.extra.rawValue + '\'') ||
             (p.key.extra.raw[0] == '\"' && p.key.extra.raw != '\"' + p.key.extra.rawValue + '\"'))
			return;
	  } 
    }

on line https://github.com/babel/minify/blob/master/packages/babel-plugin-minify-constant-folding/src/index.js#L144

@jogibear9988
Copy link
Contributor

In polymer cli i also get this code:

 var identifier = "(?:\\\\.|[\\w-]|[^\0-\\xa0])+";

from jquery to break: https://github.com/Polymer/polymer-cli/issues/886

boopathi added a commit that referenced this issue Nov 18, 2017
+ t.valueToNode(<objectExpressionNode>) returns an object expression
node and again but with a few other effects - it removes the quotes from
property names for some unicode which is not supported in Safari and a
few other older browsers. So we bail here and let propertyLiterals
plugin handle the conversion of StringLiterals to Identifiers.
+ Fix #627
+ Fix #706
boopathi added a commit that referenced this issue Nov 18, 2017
+ t.valueToNode(<objectExpressionNode>) returns an object expression
node and again but with a few other effects - it removes the quotes from
property names for some unicode which is not supported in Safari and a
few other older browsers. So we bail here and let propertyLiterals
plugin handle the conversion of StringLiterals to Identifiers.
+ Fix #627
+ Fix #706
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

9 participants