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

Add test for es6 arrow functions #1794

Merged
merged 1 commit into from Jan 18, 2016
Merged

Conversation

@vincentriemer
Copy link
Contributor

@vincentriemer vincentriemer commented Dec 4, 2015

Following the conventions of the ES6 Generators test, added a new test for the ES6 arrow function syntax.

I manually tested the code within the try-catch block and it succeeds in the latest Chrome, Firefox, and Edge but fails in the latest Safari which lines up with this support table.

@patrickkettner
Copy link
Member

@patrickkettner patrickkettner commented Dec 4, 2015

could you using eval('()=>{}')? shave a few bits

@vincentriemer
Copy link
Contributor Author

@vincentriemer vincentriemer commented Dec 5, 2015

I'm fine with that, in fact that was what I originally planned to do but I came across the generator test code decided instead to follow an existing pattern instead.

@mikach is there any reason why you went with a Function constructor instead of using eval?

@patrickkettner
Copy link
Member

@patrickkettner patrickkettner commented Jan 18, 2016

@vincentriemer sorry this got lost in the holidays!

could you squish down to a single commit?

@vincentriemer vincentriemer force-pushed the vincentriemer:master branch from 88ecee7 to ad88d19 Jan 18, 2016
@vincentriemer
Copy link
Contributor Author

@vincentriemer vincentriemer commented Jan 18, 2016

Done!

patrickkettner added a commit that referenced this pull request Jan 18, 2016
Add test for es6 arrow functions
@patrickkettner patrickkettner merged commit 71b49f5 into Modernizr:master Jan 18, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@patrickkettner
Copy link
Member

@patrickkettner patrickkettner commented Jan 18, 2016

thanks!

@kamilbrk
Copy link

@kamilbrk kamilbrk commented Mar 16, 2017

Apologies for commenting in the old thread, just want to comment on what happened with our product today due to this change.

We use CI and gulp-modernizr, which depends on customizr, which in the end depends on Modernizr through npm. Thing is, that 3rd level dependency is not locked to a specific version, so the 3.4.0 release from yesterday evening slipped into our product through CI.

This check for arrow functions is defined as arrow, so far so good.

It adds Modernizr.arrow and arrow class to <html> element, as expected, right?

I know that I could have added a custom prefix for Modernizr CSS classes on <html>, but since I didn't, it did style my entire app (<html> element) as an arrow, because we had some .arrow {} selectors in CSS.

PSA: Please check your styles, please check your CI configs, consider Yarn instead of NPM to lock down deep dependencies, etc. We spent quite a while trying to figure out why our entire web app is wrapped in a tiny box, even though QA gave us a green light during our testing for 2 weeks (until yesterday).

@ryanseddon
Copy link
Member

@ryanseddon ryanseddon commented Mar 16, 2017

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

Successfully merging this pull request may close these issues.

None yet

4 participants