Changing typeof check to a check for apply (#681) #787

Merged
merged 1 commit into from Mar 14, 2014

Conversation

Projects
None yet
4 participants
@ccummings
Contributor

ccummings commented Mar 14, 2014

One of the methods that goes through this code path is a RegExp and in certain older browsers, RegExp is callable meaning using typeof on one returns'function'. So it passes the typeof check and when we try to use apply on the RegExp it throws an error.

You can see bugs to fix this weirdness in all major browsers.

The fix is to check method for apply instead of using a typeof check.

Closes #681

ccummings added a commit that referenced this pull request Mar 14, 2014

Merge pull request #787 from bitovi/681-android-can-route-error
Changing typeof check to a check for apply (#681)

@ccummings ccummings merged commit d8a5d1a into master Mar 14, 2014

1 check was pending

default The Travis CI build is in progress
Details

@ccummings ccummings deleted the 681-android-can-route-error branch Mar 14, 2014

@daffl daffl added this to the 2.0.6 milestone Mar 14, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 14, 2014

Contributor

We should not really close this without testing against these browsers. I think once lazy map is done, we should add testing in android, iOS, and other browsers.

Sent from my iPhone

On Mar 13, 2014, at 9:26 PM, Curtis Cummings notifications@github.com wrote:

One of the methods that goes through this code path is a RegExp and in certain older browsers, RegExp is callable meaning using typeof on one returns'function'. So it passes the typeof check and when we try to use apply on the RegExp it throws an error.

You can see bugs to fix this weirdness in all major browsers.

The fix is to check method for apply instead of using a typeof check.

Closes #681

You can merge this Pull Request by running

git pull https://github.com/bitovi/canjs 681-android-can-route-error
Or view, comment on, or merge it at:

#787

Commit Summary

Changing typeof check to a check for apply
File Changes

M route/route.js (2)
Patch Links:

https://github.com/bitovi/canjs/pull/787.patch
https://github.com/bitovi/canjs/pull/787.diff

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Mar 14, 2014

We should not really close this without testing against these browsers. I think once lazy map is done, we should add testing in android, iOS, and other browsers.

Sent from my iPhone

On Mar 13, 2014, at 9:26 PM, Curtis Cummings notifications@github.com wrote:

One of the methods that goes through this code path is a RegExp and in certain older browsers, RegExp is callable meaning using typeof on one returns'function'. So it passes the typeof check and when we try to use apply on the RegExp it throws an error.

You can see bugs to fix this weirdness in all major browsers.

The fix is to check method for apply instead of using a typeof check.

Closes #681

You can merge this Pull Request by running

git pull https://github.com/bitovi/canjs 681-android-can-route-error
Or view, comment on, or merge it at:

#787

Commit Summary

Changing typeof check to a check for apply
File Changes

M route/route.js (2)
Patch Links:

https://github.com/bitovi/canjs/pull/787.patch
https://github.com/bitovi/canjs/pull/787.diff

Reply to this email directly or view it on GitHub.

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Mar 14, 2014

Contributor

I ran the tests on Android 2.2, 2.3, 4.0, 4.1 and 4.2 using Browserstack to verify the fix.

Contributor

ccummings commented Mar 14, 2014

I ran the tests on Android 2.2, 2.3, 4.0, 4.1 and 4.2 using Browserstack to verify the fix.

@dfyx

This comment has been minimized.

Show comment
Hide comment
@dfyx

dfyx Mar 14, 2014

Wouldn't it be better to have both checks? In the future we could encounter objects that have an apply method that does something completely different.

dfyx commented Mar 14, 2014

Wouldn't it be better to have both checks? In the future we could encounter objects that have an apply method that does something completely different.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 14, 2014

Contributor

I mean @daffl needs to hook us up with automated testee testing of android, iOS, and other browsers.

Contributor

justinbmeyer commented Mar 14, 2014

I mean @daffl needs to hook us up with automated testee testing of android, iOS, and other browsers.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 14, 2014

Contributor

That sounds like a fantastic plan 👍
We just need to figure out how frequently to run those (nightly builds?) running all browsers with every build will make them take forever.

Contributor

daffl commented Mar 14, 2014

That sounds like a fantastic plan 👍
We just need to figure out how frequently to run those (nightly builds?) running all browsers with every build will make them take forever.

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