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

Assume Object/Array prototypes are sealed #822

Closed
wants to merge 3 commits into from
Closed

Conversation

@jamesrdf
Copy link

jamesrdf commented Jun 4, 2012

This patch replaces ecma-5.js with utilities functions in tree.js. This allows LESS to run in non ECMA-5 environments where Object, Array, and Array.prototype are sealed.

@neonstalwart

This comment has been minimized.

Copy link
Contributor

neonstalwart commented Jun 4, 2012

fwiw, this would fix #99 which for some reason got closed without a fix

@lukeapage

This comment has been minimized.

Copy link
Member

lukeapage commented Jul 24, 2012

Hi,

Excuse my ignorance (I really haven't investigated), what environments have sealed object and array prototypes?

And which old javascript engines have bugs that mean replace || with :?

Also the pull appears (at a cursery glance) to include other changes (e.g. https://github.com/cloudhead/less.js/pull/822/files#L2L900) or am I wrong?

@jamesrdf

This comment has been minimized.

Copy link
Author

jamesrdf commented Jul 25, 2012

On Mac OS X 10.7.4 (Lion) with Java(TM) SE Runtime Environment (build 1.6.0_33-b03-424-11M3720) and Java HotSpot(TM) 64-Bit Server VM (build 20.8-b03-424, mixed mode), in the included Language ECMAScript 1.6 implemention "Mozilla Rhino" 1.6 release 2 the following is true.

typeof(undefined || null) == "undefined"

However, in most ECMAScript implementations that would be false. This causes some issues with less.js when a undefined value is passed through a "var || null" expression. So I replaced all || and && expressions that did not evaluate to a boolean.

Those changes you point out are in the artifact less-1.3.0.js, which was modified when I ran "make". It looks like the artifact was not update with the current parser.js. So this pull requests also bring the artifact update (not just with these changes, but also with the entire source code base). I am not sure what the policy is for artifacts, but I am happy to exclude them if desired.

@@ -897,14 +781,38 @@ less.Parser = function Parser(env) {
elements.push(new(tree.Element)(c, e, i));
c = $('>');
}
$('(') && (args = $(this.entities.arguments)) && $(')');
if ($('(')) {

This comment has been minimized.

Copy link
@jamesrdf

jamesrdf Jul 25, 2012

Author

This comes from 1857b7c#L0R763

@dmcass

This comment has been minimized.

Copy link
Contributor

dmcass commented Aug 6, 2012

Array/Object prototypes are not sealed in Rhino 1.6r2, so there's still a question of whether or not this PR is necessary for it's title purpose. I'd prefer not to get into the debate point that was brought up in #99 (extending natives), and I'm assuming @cloudhead didn't want to either since he closed it without comment.

Regarding the other changes (changing default operators to ternary operators), this should probably be done in a different PR, since once again it's not related to the ES5 changes. In addition, this was fixed in Rhino 1.6r3. Rhino 1.6r2 is nearing 7 years old, is it still widely used enough to warrant supporting it?

Dist files should definitely be excluded from any PR since merging a PR does not initiate a new version or build.

@cloudhead

This comment has been minimized.

Copy link
Member

cloudhead commented Aug 7, 2012

sorry, but no.

@cloudhead cloudhead closed this Aug 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.