-
Notifications
You must be signed in to change notification settings - Fork 3
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
do es-shims API #3
Conversation
return _entries(obj).map(fn).reduce(_foldToObject, {}); | ||
} | ||
|
||
function _entries(obj) { |
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.
¯\_(ツ)_/¯ you could use https://npmjs.com/object.entries for this too
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.
Object.entries
gets only enumerable properties, and I want all of them.
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.
ah, gotcha
this._provides = provides; | ||
this._staticProvides = staticProvides; | ||
// TODO: check that there's no conflicts between requires/staticRequires/provides/staticProvides | ||
Object.assign( |
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.
you also may want to use a babel transform for this to https://npmjs.com/object.assign, like https://github.com/airbnb/react-dates/blob/master/.babelrc#L7 (that way it will work down to ES5; even if someone's shammed Symbol)
return _entries(obj).map(fn).reduce(_foldToObject, {}); | ||
} | ||
Object.defineProperties(implementation, { | ||
implementation: { value: implementation, enumerable: false, configurable: true, writable: false }, |
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.
https://npmjs.com/define-properties will do this for you, with a fallback for when Object.defineProperties isn't supported
@@ -0,0 +1,7 @@ | |||
let global = Function('return this')(); |
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 think here you can just use global
; a bundler will convert it appropriately for you - unless you're worried about someone reassigning the global global.
import implementation from './implementation'; | ||
|
||
export default function() { | ||
let p = global.Protocol; |
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.
however, doing just Protocol
might be more robust than pulling it off global
?
24fc57f
to
6a7c255
Compare
6a7c255
to
2986545
Compare
} | ||
|
||
|
||
export default class Protocol extends null { |
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.
Classes should not extend null
as things stand. TC39 does not have consensus for what the semantics should be, and the current semantics mean that such classes cannot be instantiated. See tc39/ecma262#1036, tc39/ecma262#781, the notes, etc.
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.
Fixed: 7571e39
2986545
to
ba90e4a
Compare
function _entries(obj) { | ||
let stringKeys = Object.getOwnPropertyNames(obj); | ||
let symKeys = Object.getOwnPropertySymbols(obj); | ||
return [...stringKeys, ...symKeys].map(p => [p, obj[p]]); |
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.
In that case, Array.from(stringKeys, p => [p, obj[p]]).concat(Array.from(symKeys, p => [p, obj[p]]))
would be slightly more efficient
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.
It's a reference implementation.
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.
fair enough :-) altho i'd say if it's published on npm, then it's not a reference implementation - it's a production-usable package.
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.
Hoooo boy that is a low bar. 99.9% of the code on npm is unusable garbage that you should never ever put into production.
No description provided.