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

Update shape.js #138

Closed
wants to merge 2 commits into from
Closed

Update shape.js #138

wants to merge 2 commits into from

Conversation

dbleier
Copy link

@dbleier dbleier commented Aug 13, 2018

changed function isEven(num) to isEven = function(num) for older browser support

@justinbmeyer
Copy link
Contributor

Thanks. There's a browser that doesn't support named functions?

@phillipskevin
Copy link
Contributor

Strict mode doesn't like the function declared inside the block. @dbleier gave the stack trace on gitter.

@phillipskevin
Copy link
Contributor

@dbleier I think the saucelabs tests are failing because your fork doesn't have access to the environment variables used to connect to saucelabs. I'll figure out if there's some way around this tomorrow, but for now I've put in a separate PR: #140.

@dbleier
Copy link
Author

dbleier commented Aug 14, 2018

@phillipskevin thanks

@phillipskevin
Copy link
Contributor

I released that fix in can-reflect@1.17.3.

@justinbmeyer
Copy link
Contributor

@phillipskevin would we have seen this if we also put strict mode in the file? Perhaps we should have to "test" that the code is working in strict mode?

@dbleier
Copy link
Author

dbleier commented Aug 14, 2018

@phillipskevin thanks
@justinbmeyer I thought canjs always was supposed to run in strict mode?

@phillipskevin
Copy link
Contributor

"use strict"; is in the file. I'm not sure why this error is only thrown by a specific browser.

We would have caught this if jshint was running correctly. I opened #139 to fix this.

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.

None yet

3 participants