Skip to content

Make "column"-property of Errors enumerable#1285

Merged
nknapp merged 2 commits intohandlebars-lang:4.xfrom
nknapp:4.x
Dec 30, 2016
Merged

Make "column"-property of Errors enumerable#1285
nknapp merged 2 commits intohandlebars-lang:4.xfrom
nknapp:4.x

Conversation

@nknapp
Copy link
Copy Markdown
Collaborator

@nknapp nknapp commented Dec 20, 2016

This PR contains a test case for #1284. It verifies that the column-property is included in the compilation error. The test passes. This is not mean to be merged right now, primarily for discussion.

@nknapp nknapp changed the title WIP: Add test for #1284 Make "column"-property of Errors enumerable Dec 22, 2016
@nknapp
Copy link
Copy Markdown
Collaborator Author

nknapp commented Dec 22, 2016

I have reworded my commit messages and would not consider this work-in-progress anymore. If all tests pass and this PR does not introduce another regression (I have no Safari to test the bug fixed in 20c965c), it could be merged (from my point of view) and it would probably help @nathanboktae a lot if there was a 4.0.7 release including this PR.

@kpdecker @lawnsea @ErisDS @wycats

@nathanboktae
Copy link
Copy Markdown

I have no Safari to test

That's what SauceLabs is for right? I'm not seeing right off hand if Travis CI is kicking off tests in SauceLabs

@nknapp
Copy link
Copy Markdown
Collaborator Author

nknapp commented Dec 29, 2016

@wycats gave me commit access tonight. I have created a local branch "tmp/stage-4.0.7" and push the changes there. This should run the tests in SauceLabs and show any Safari-problems.

@nknapp
Copy link
Copy Markdown
Collaborator Author

nknapp commented Dec 29, 2016

Tests case failing for Safari 8: '97' should === '5': Checking error column".

Why should Safari 8 return a completely different column than all other browsers?

@nknapp
Copy link
Copy Markdown
Collaborator Author

nknapp commented Dec 29, 2016

Ah, that's why: jquery/esprima#1290 (comment)

Related to handlebars-lang#1284

The test ensures that the property is there, because it is important to
some people.
Fixes handlebars-lang#1284

Appearently, there is a use-case of stringifying the error in order to
evaluated its properties on another system. There was a regression
from 4.0.5  to 4.0.6 that the column-property of compilation errors
was not  enumerable anymore in 4.0.6 (due to  commit 20c965c) and
thus was not included in the output of "JSON.stringify".
@nknapp
Copy link
Copy Markdown
Collaborator Author

nknapp commented Dec 30, 2016

The column-property is read-only in Safari 8, its value won't change, even with Object.defineProperty. The test case now omits the column-check, if the property is read-only.

@nknapp nknapp merged commit a023cb4 into handlebars-lang:4.x Dec 30, 2016
nknapp added a commit that referenced this pull request Jan 1, 2019
- this is the case in Safari starting with version 9.0
- see 07511e0
- also see #1285
nknapp added a commit that referenced this pull request Jan 1, 2019
- this is the case in Safari starting with version 9.0
- see 07511e0
- also see #1285
nknapp added a commit that referenced this pull request Jan 1, 2019
- this is the case in Safari starting with version 9.0
- see 07511e0
- also see #1285
nknapp added a commit that referenced this pull request Jan 2, 2019
- this is the case in Safari starting with version 9.0
- see 07511e0
- also see #1285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants