-
Notifications
You must be signed in to change notification settings - Fork 2
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
IE11 fixes #55
IE11 fixes #55
Conversation
var hydratedValue = hydrateValue(value["$not"], unknownHydrator); | ||
var typeName = hydratedValue.constructor.name; | ||
var hydratedValue = hydrateValue(value.$not, unknownHydrator); | ||
var typeName = hydratedValue.constructor.name || hydratedValue.constructor.toString().match(/^\s*function\s*(\S*)\s*\(/)[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hack to get IE 11 working because constructor.name
isn’t available.
I think we should probably fix canjs/can-reflect#167 and add a getConstructorName
method to can-reflect that returns just the constructor name (instead of something like ConstructorName{}
, which is what getName
returns).
Otherwise, we could maybe refactor this code to not rely on the constructor name… I think that would be a lot more involved though because there’s a lot of functions involved with the hydrateValue()
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the problem with getName
? I'm not sure I follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that you can pass in a constructor function to getName
and it’ll return just the name, not something like ConstructorName{}
. Scratch what I said about adding a separate getConstructorName
method.
This works around constructor names not being available in IE. Closes #54
f0a263e
to
5256863
Compare
doc/examples/date-string-example.js
Outdated
@@ -1,4 +1,4 @@ | |||
import {DefineMap, QueryLogic, Reflect as canReflect} from "can"; | |||
import {DefineMap, QueryLogic } from "can"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a space at the end and not one at the beginning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find + replace mistake, fixed now.
@@ -124,9 +124,11 @@ QUnit.test("returns undefined against incompatible set", function(assert) { | |||
var fromQuery = new BasicQuery({ | |||
filter: new BasicQuery.KeysAnd({ type: 'critical' }) | |||
}); | |||
var res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the indenting is off here. Also the next line could be try {
(with a space) while you're fixing this up elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a few comments about cosmetic things and one question. Nothing that should merging. Nice work.
This PR has a few things: