describe a few compiler assumptions #2883

Skalman · 2018-02-04T22:21:18Z

I've created a new README section to describe some of the compiler assumptions. I'd appreciate any suggestions of clarifications.

In particular, I'm unsure about the factual accuracy of the last couple of bullet points:

Getting and setting properties on a plain object does not cause other side effects (using .watch() or Proxy).
Object properties are able to be added (not prevented with Object.defineProperty(), Object.defineProperties(), Object.freeze() or Object.preventExtensions()).

kzc · 2018-02-04T22:56:52Z

Since you're editing the README, could you please drop this in the Source Map section?

#2876 (comment)

Edit: perhaps under a new section called "Source Maps and Debugging" at the bottom of the README.

alexlamsl · 2018-02-05T07:25:49Z

@kzc if you are going to add that source map text, please combine it with https://github.com/mishoo/UglifyJS2#uglify-fast-minify-mode - no need to have more than one place describing non-default usage.

@Skalman other than missing .seal(), the rest LGTM.

kzc · 2018-02-05T07:47:41Z

"Uglify breaks debugging with source maps" is the single most frequently created issue in bundler wrapper projects. The explanation doesn't belong in the fast minify mode section - it's completely unrelated. It's up to @Skalman if he wants to add it.

Skalman · 2018-02-05T20:31:53Z

I added the note on source maps to the bottom of the readme. Let me know if I should move it.

kzc · 2018-02-05T21:32:52Z

LGTM

alexlamsl · 2018-02-05T21:33:37Z

README.md

+  `Error.prototype.stack` to be anything in particular.
+- Getting and setting properties on a plain object does not cause other side effects
+  (using `.watch()` or `Proxy`).
+- Object properties are able to be added (not prevented with


Object properties can be added, removed and modified (or writeable as they call it).

alexlamsl · 2018-02-05T21:37:03Z

README.md

+are known to have an adverse effect on debugging with source maps. This is
+expected as code is optimized and mappings are often simply not possible as
+some code no longer exists. For highest fidelity in source map debugging
+disable the Uglify `compress` option and just use `mangle`.


Just combine this with Uglify Fast Minify Mode above - no point having a seperate section talking about non-default option usage.

Again, it is unrelated to Uglify Fast Minify. This source map debugging section would benefit with its own heading so it is externally linkable.

@alexlamsl, I'm not sure I think that makes sense. That section doesn't include anything about source maps nor debugging.

To me it seems like multiple sections under "Miscellaneous" talk about non-default option usage, including Using native Uglify AST with minify(), ESTree / SpiderMonkey AST, and Use Acorn for parsing.

Makes more sense to mention under Uglify Fast Minify that when somebody is terminally confused by a debugger, this option would also be useful.

I am against text dedicated to non-default usage in general because it will cause some people go for it based on misplaced beliefs, e.g. keep_infinity in #2743 - and then in return we get weird complaints when things don't work the way they expect it to, e.g. #2856

Do you mean that you're against any information on source maps? If the text were to be combined with an existing section, I'd suggest "CLI source map options".

Options are provided and documented so that people can use them as they see fit. Source map use with uglify is extremely common - particularly when Uglify is used with bundlers.

This paragraphs warrants it own section with a heading as it is a frequently asked question. It may be underneath Uglify Fast Minify with a smaller heading if you prefer.

Do you mean that you're against any information on source maps?

Not at all - just not worth attracting unwarranted attention to it.

Source map use with uglify is extremely common.

Confusion when using a debugger isn't as common amongst programmers.

It may be underneath Uglify Fast Minify with a smaller heading if you prefer.

That would at least be better - thanks.

kzc · 2018-02-05T22:34:23Z

README.md

+
+### Compiler assumptions
+
+To allow for better optimizations, the compiler makes various assumptions.


Please replace the period at the end of the line with a colon.

kzc · 2018-02-05T22:35:58Z

README.md

+
+To allow for better optimizations, the compiler makes various assumptions.
+
+- `.toString()` and `.valueOf()` have not been overridden, and don't have side effects.


toString() can be overridden in new functions/classes - it just cannot have side effects.

Do you have a suggestion for how to phrase this? My take:

.toString() and .valueOf() don't have side effects, and for built-in objects they have not been overridden.

... only if unsafe is not specified, though I guess that's covered under https://github.com/mishoo/UglifyJS2#the-unsafe-compress-option

a0d408e is good.

kzc · 2018-02-05T22:53:42Z

The section on unsafe is quite out of date. It should mention that the unsafe compress option is actually considered to be safe for sane code that does not modify builtin JavaScript functions and classes. For example, unsafe is not suitable for polyfills.

It also would be good to demonstrate an advanced unsafe test case in that section. For example:

$ cat test.js
var s = [ "C", "A", "T" ].join("_");
console.log(s.replace(/_/g, ""));

$ bin/uglifyjs test.js --toplevel -c unsafe,passes=2
console.log("CAT");

Skalman · 2018-02-05T22:58:23Z

I'm not planning on adding information about unsafe at this point, but I agree that an update would be nice.

kzc · 2018-02-05T23:00:41Z

I'm not planning on adding information about unsafe at this point, but I agree that an update would be nice.

No worries. Leave that for someone else.

kzc · 2018-02-05T23:07:15Z

@alexlamsl OT - do you recall why these two functions were not included with unsafe?

$ echo 'console.log("Abc".toUpperCase(), "Xyz".toLowerCase());' | node0_10_41 
ABC xyz

$ echo 'console.log("Abc".toUpperCase(), "Xyz".toLowerCase());' | bin/uglifyjs -c unsafe
console.log("Abc".toUpperCase(),"Xyz".toLowerCase());

They both are available in node 0.10.

Do they throw or have some side effect or something?

kzc · 2018-02-05T23:10:58Z

Perhaps toUpperCase and toLowerCase are affected by the locale?

alexlamsl · 2018-02-05T23:11:05Z

@kzc Turkish i? 😏

Edit: looks like you've just figured it out 😉

Skalman · 2018-02-05T23:19:25Z

If I understand the documentation on MDN, toUpperCase/toLowerCase shouldn't be affected by the locale.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toLowerCase
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/toLocaleLowerCase

kzc · 2018-02-05T23:23:31Z

They even mention Turkish explicitly :-)

The toLocaleLowerCase() method returns the value of the string converted to lower case according to any locale-specific case mappings. toLocaleLowerCase() does not affect the value of the string itself. In most cases, this will produce the same result as toLowerCase(), but for some locales, such as Turkish, whose case mappings do not follow the default case mappings in Unicode, there may be a different result.

alexlamsl · 2018-02-05T23:36:57Z

I remember seeing polyfill for toLoweCase() before. In any case, just found a list of these characters:
https://gist.github.com/barlas/760cbf77b31c6922d159

I'll test them on various platforms after some sleep – and if it turns out they all work as expected, then a PR for unsafe evaluate.

describe a few compiler assumptions

48ce5b0

Skalman added 2 commits February 5, 2018 21:24

add not one Object.seal()

b3128f0

add note on debugging with source maps

46f893b

alexlamsl reviewed Feb 5, 2018

View reviewed changes

Skalman added 2 commits February 5, 2018 22:44

"Object properties can be added, removed and modified"

90508c6

move up section on debugging with source maps

600ed69

kzc reviewed Feb 5, 2018

View reviewed changes

toString() and valueOf() rephrasing for accuracy

a0d408e

alexlamsl approved these changes Feb 5, 2018

View reviewed changes

alexlamsl merged commit cb0257d into mishoo:master Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

describe a few compiler assumptions #2883

describe a few compiler assumptions #2883

Skalman commented Feb 4, 2018

kzc commented Feb 4, 2018 •

edited

Loading

alexlamsl commented Feb 5, 2018

kzc commented Feb 5, 2018

Skalman commented Feb 5, 2018

kzc commented Feb 5, 2018

alexlamsl Feb 5, 2018

alexlamsl Feb 5, 2018

kzc Feb 5, 2018

Skalman Feb 5, 2018

alexlamsl Feb 5, 2018

Skalman Feb 5, 2018

kzc Feb 5, 2018

alexlamsl Feb 5, 2018

kzc Feb 5, 2018

kzc Feb 5, 2018

Skalman Feb 5, 2018

alexlamsl Feb 5, 2018

kzc Feb 5, 2018

kzc commented Feb 5, 2018

Skalman commented Feb 5, 2018

kzc commented Feb 5, 2018

kzc commented Feb 5, 2018

kzc commented Feb 5, 2018

alexlamsl commented Feb 5, 2018 •

edited

Loading

Skalman commented Feb 5, 2018

kzc commented Feb 5, 2018

alexlamsl commented Feb 5, 2018 •

edited

Loading


		### Compiler assumptions

		To allow for better optimizations, the compiler makes various assumptions.


		To allow for better optimizations, the compiler makes various assumptions.

		- `.toString()` and `.valueOf()` have not been overridden, and don't have side effects.

describe a few compiler assumptions #2883

describe a few compiler assumptions #2883

Conversation

Skalman commented Feb 4, 2018

kzc commented Feb 4, 2018 • edited Loading

alexlamsl commented Feb 5, 2018

kzc commented Feb 5, 2018

Skalman commented Feb 5, 2018

kzc commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Feb 5, 2018

Skalman commented Feb 5, 2018

kzc commented Feb 5, 2018

kzc commented Feb 5, 2018

kzc commented Feb 5, 2018

alexlamsl commented Feb 5, 2018 • edited Loading

Skalman commented Feb 5, 2018

kzc commented Feb 5, 2018

alexlamsl commented Feb 5, 2018 • edited Loading

kzc commented Feb 4, 2018 •

edited

Loading

alexlamsl commented Feb 5, 2018 •

edited

Loading

alexlamsl commented Feb 5, 2018 •

edited

Loading