bublejs / buble

https://buble.surge.sh
MIT License
869 stars 67 forks source link

fix #149 Non string keys #187

Closed exe-dealer closed 5 years ago

adrianheine commented 5 years ago

That looks very good. Are you interested in adding some tests? Otherwise I'd merge this as it is and add tests myself.

adrianheine commented 5 years ago

Thanks! I'd like to have the commits squashed into one and with a commit message that's a somewhat complete phrase and starts with an upper-case letter. I also need to set up travis integration again after moving the repo to the new bublejs organization. You're code changes are good to merge from my point of view, though :)

exe-dealer commented 5 years ago

what about to replace String(expr) to ""+expr ? Last code is uglier but its protected from String redeclaration

adrianheine commented 5 years ago

I think I would prefer String, since it is prettier and performs the correct operation (toString, not valueOf).

class X {
  valueOf() { return "valueOf" }
  toString() { return "toString" }
}

console.log(
  { [new X()]: "new X()" },
  (new X()) + "",
  String(new X()),
  { valueOf: "valueOf", toString: "toString" }[new X()]
)
adrianheine commented 5 years ago

Ok, I merged this. Thanks for your patience!