Skip to content

Add wrapper for typeof operator #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

Merged
merged 1 commit into from
Sep 13, 2015
Merged

Add wrapper for typeof operator #2

merged 1 commit into from
Sep 13, 2015

Conversation

flimzy
Copy link
Member

@flimzy flimzy commented Sep 8, 2015

This is a work in progress, and not yet ready to merge.

I'm curious whether this is a good idea. I remember reading somewhere (I can't find it now) that Richard suggested "manual" reflection on js.Objects to determine their type (i.e. checking for the existence of object methods as evidence for the object's type). The approach in this PR seems far easier to me when dealing with primitive types, and I suspect more efficient in many cases.

My questions:

  1. Is it even a good idea to expose JS's typeof to Go? Perhaps this has been discussed somewhere I haven't seen.
  2. If it is good/useful, is my approach the best one? (i.e. wrapping the typeof operator in a function which is evaled). The eval runs only once, so the normal efficiency concerns of 'eval' shouldn't matter too much here. But perhaps there are other drawbacks I haven't considered.

For background, the reason I care about this is that I have a jQuery Mobile event handler which passes either a DOM object or a URL string as one argument to a particular callback. At the moment, I'm determining the object type by comparing its .String() output to [object Object], which works in my case (since a URL ought never equal that string), but direct access to typeof seems more fool-proof, and much simpler than writing complex heuristics in my code.

If it is determined that this approach is basically sound, I still need to add additional error checking before merging. It's presently easy to crash the Typeof() function by passing certain JS-unfriendly data structures (e.g. map[string]string{} or even js.Object{})

@flimzy
Copy link
Member Author

flimzy commented Sep 12, 2015

This is now ready to my personal satisfaction. I'd love to hear feedback from someone else, if only to say "do what you want with this repo." :)

@dmitshur
Copy link
Member

This is now ready to my personal satisfaction. I'd love to hear feedback from someone else, if only to say "do what you want with this repo." :)

As I continue to have no plans to use JS builtin funcs, unfortunately that is the only thing I can say here: I don't have any specific comments. (This doesn't mean there's no one else who would find this useful, and a small chance I will find this useful in the future.)

@@ -53,7 +54,7 @@ and its current state in this package.
Get or update this package and dependencies with:

```
go get -u github.com/gopherjs/jsbuiltin
gopherjs get github.com/gopherjs/jsbuiltin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest avoiding relying on gopherjs get. See gopherjs/gopherjs#270.

Instead, you can opt to go for:

go get -u -d -tags=js github.com/gopherjs/jsbuiltin

@flimzy
Copy link
Member Author

flimzy commented Sep 13, 2015

In the absence of (to my knowledge, anyway) any official developer guidelines, I prefer to exercise caution with my commit access, even though I'm the only one who's done any work on this repo. It no longer bears only my name, so I feel caution is the responsible course.

On the other hand, I also don't want to burden you, @shurcooL, or anyone else.

I'll go ahead and merge this, as it is something I will use immediately, and can always be tweaked later if other feedback comes in at a later date.

flimzy added a commit that referenced this pull request Sep 13, 2015
Add wrapper for `typeof` operator
@flimzy flimzy merged commit ac7b7eb into gopherjs:master Sep 13, 2015
@flimzy flimzy deleted the typeof branch September 13, 2015 04:20
@dmitshur
Copy link
Member

That's a good way to approach it @flimzy, thanks.

@dmitshur
Copy link
Member

Also, you're welcome to use branches for feature development in this repo, if you don't want to have to work on a fork.

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.

2 participants