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

Feature marked incorrectly for Safari 10 #1175

Closed
ssilve1989 opened this issue Sep 13, 2017 · 19 comments · Fixed by #1474
Closed

Feature marked incorrectly for Safari 10 #1175

ssilve1989 opened this issue Sep 13, 2017 · 19 comments · Fixed by #1474

Comments

@ssilve1989
Copy link

babel-preset-env, which uses this table, ends up incorrectly excluding a plugin transform-es2015-block-scoping when targeting support of Safari 10.

I believe this is due to the features const and let under bindings being marked completed but there is a bug in Safari 10, shown here where the following

{
  let a = 3
}

let a = 3

throws SyntaxError: Can't create duplicate variable that shadows a global property: 'a'

References babel/babel-preset-env#411

@afmenez
Copy link
Contributor

afmenez commented Sep 21, 2017

@chicoxyzzy, by reading the description I thought it was about writing two new subtests (for let and const), but it is now marked as "test code bug". Do you mean the existing tests should be changed to fail on the Safari buggy implementation?

@ljharb
Copy link
Member

ljharb commented Sep 22, 2017

Separate tests are good, but yes, absolutely a test should always fail on a buggy implementation.

@afmenez
Copy link
Contributor

afmenez commented Sep 22, 2017

I was not able to cram this issue on the existing subtests, so I added new ones.

@ssilve1989
Copy link
Author

ssilve1989 commented Sep 27, 2017

Just upgraded to Safari Version 11.0 (12604.1.38.1.7) today and safari still throws this error with that code snippet 😡

@afmenez
Copy link
Contributor

afmenez commented Sep 27, 2017

Yep, my patch #1183 is not merged yet.

@afmenez
Copy link
Contributor

afmenez commented Oct 9, 2017

I believe one of the reasons for not merging is that I haven't actually run the new tests on Safari. Could you do that and comment on the PR if it actually solves your issue?

@ssilve1989
Copy link
Author

So I ran the build and opened up Safari to view it. In the Safari 11 column, it lists No for scope-shadow-resolution and scope-shadow-resolution (strict mode). However in the Current Browser column they show as passing. The current browser, that i was running it on was Safari Version 11.0 (12604.1.38.1.7), and indeed it does work on that version of 11. I'm not sure why it didn't 13 days ago when I first upgraded Safari, its the same exact version as then

@afmenez
Copy link
Contributor

afmenez commented Oct 10, 2017

Yes, the result columns are hard-coded, only the "current browser" column is live. I marked it no for Safari because you said it should not work. I can change it to yes, but some questions remain:

  • How about Safari 10.1 and earlier?
  • If they work too, is the test incorrect?

@ssilve1989
Copy link
Author

ssilve1989 commented Oct 10, 2017

So since I've upgraded my MacOS, the only easy way I have to test lower Safari versions is through crossbrowsertesting.com, and through that, Safari 10 is actually showing as passing on Version 10.1.2 (12603.3.8), and that is accurate as running the snippet in that console doesn't throw an exception. Unfortunately I did not record the build number of the Version 10 that did throw the error so I can't tell if this is the same and something bizarre is happening, or it's a different build where the fix was implemented.

@afmenez
Copy link
Contributor

afmenez commented Oct 10, 2017

I am starting to believe the test is incomplete. Did you check the code I implemented to see if it should triggers the problem you refer to?

@ssilve1989
Copy link
Author

The code for your tests looks fine. What's troubling is that running code like that, or the snippet at the start of this thread, in the Safari 10 version crossbrowsertesting.com supplies, works without any errors. So I'm thinking the version 10 I had used where the error did occur must have been a different build or patch version, but I no longer have access to find out what that version was.

So in short, as far as I can tell, shadow scoping is now not throwing errors and working as expected in Safari 10+

@afmenez
Copy link
Contributor

afmenez commented Oct 10, 2017

OK, I'll fix that in my PR. But I believe you mean 10.1+, right?

@ssilve1989
Copy link
Author

Correct, 10.1.2 to be precise

@afmenez
Copy link
Contributor

afmenez commented Oct 10, 2017

OK, I updated the patch to reflect 10.1 results. Now the only version where this makes a difference is 10.0. Can you test it to check if this test is still necessary?

@ssilve1989
Copy link
Author

I don't currently have access to that version. It seems crossbrowsertesting.com only supplies the latest version that comes with a particular MacOS.

@afmenez
Copy link
Contributor

afmenez commented Jan 15, 2019

Since #1183 was merged, I believe this issue can now be closed.

@wessberg
Copy link

@afmenez, I would suggest that we reopen this issue. #1183 was merged, but in it, only Safari 10 and not 10.1 was marked as having this bug. See https://github.com/kangax/compat-table/blob/gh-pages/data-es6.js#L561.

However, Safari 10.1 too has the bug. I’ve made a simple repro: http://safari_10_1_repro.surge.sh/ that includes the following piece of code:

function func(a = 1) {
    for (let a = 2; ;) {
        break;
    }
}
func();

I’ve tested it in Safari 10.1 and can confirm that it doesn’t work. We get SyntaxError: Cannot declare a let variable twice ‘a’.

Screen Shot 2019-06-23 at 12 38 38 PM

@chicoxyzzy
Copy link
Collaborator

Is it Safari 10.1.0?

@chicoxyzzy chicoxyzzy reopened this Jun 23, 2019
@wessberg
Copy link

I tested in Safari 10.1.2 (12603.3.8).
So, at least the range from Safari 10.0 to 10.1.2 is having the bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants