Current coverage is 85.11%

Merging #3327 into master will decrease coverage by -0.27% as of 44b2edc

@@            master   #3327   diff @@
======================================
  Files          215     215       
  Stmts        15768   15769     +1
  Branches      3385    3386     +1
  Methods          0       0       
======================================
- Hit          13463   13422    -41
- Partial        684     726    +42
  Missed        1621    1621       

Review entire Coverage Diff as of 44b2edc

Powered by Codecov. Updated on successful CI builds.

Are there other transforms that could take this option as well?

cc @amasad thoughts? Sounds fine to me

I think the use of polyfill here is confusing. Polyfill usually means that we are extending builtins which is not the case here, we are just using a babel helper. How about changing the wording and the option to useObjectAssign (which defaults to false).

Ok, and do we want to be more general like useNative or useBuiltIn - so we can have a convention for other plugins or specific to useObjectAssign?

I'm not crazy about polyfill either. If this is going to be a generic thing (e.g. @hzoo's naming suggestion) maybe it'd be a good idea to also make the value an object, since there could be more than one native or built-in in play in a plugin right? E.g.: useNative: {objectAssign: true}.

const helper = file.opts.useNative && file.opts.useNative.objectAssign ?
  t.memberExpression(t.identifier('Object'), t.identifier('assign')) :
  file.addHelper("extends");

+1 for useBuiltin.
useNative: {objectAssign: true} goes against my YAGNI instinct 😛

I'm ok with any of those. Being more specific feels definitely much better
:). I'm wondering if it would be a good thing to have these sort of options
somewhat global though. After all, if I'm choosing to directly use
Object.assign somewhere it's vey likely that I'll want to do it everywhere
else too. What do you think about that?
On Mon 8 Feb 2016 at 22:43 Amjad Masad notifications@github.com wrote:

+1 for useBuiltin.
useNative: {objectAssign: true} goes against my YAGNI instinct [image:
😛]

—
Reply to this email directly or view it on GitHub
#3327 (comment).

That would be #3331 - we would still do the option per plugin though

Good stuff! Want me to update the PR accordingly then?

Maybe we can get some more opinions @loganfsmyth?

Sounds like either useBuiltIn: true or useBuiltIn: { objectAssign: true } for future proof although YAGNI.

Although we can always override it later so you can do both? I say just useBuiltIn: true is fine for now and we can always add in support for an object later if we actually need it?

@amasad @hzoo

useNative: {objectAssign: true} goes against my YAGNI instinct 😛

I hear you 😃 I thought we should consider what might be the case in other plugins before deciding what to do here, since the point of the name change is to generalize it for all plugins. Personally it wouldn't bother me to be more explicit + future proof, but if you all don't think that's likely to be an issue, that's cool. And as you noted @hzoo, yep, there's an escape hatch if it turns out to be necessary:

Although we can always override it later so you can do both? I say just useBuiltIn: true is fine for now and we can always add in support for an object later if we actually need it?

I'd be in favor of useBuiltIn: true or maybe better useBuiltIns: true (with an s) since in the general case many builtins may be in use. We should as explicit as possible about the type so we can overload it in the future if we want.

 const {useBuiltin = false} = file.opts;
 if (typeof useBuiltin !== "boolean") throw new Error("Option 'useBuiltin' for transform-object-rest-spread must be a boolean.");

ping @dariocravero - if you're busy I can finish up the changes

Although now that I look at the helper.. it already checks for Object.assign?

Or do you just not want to include the helper, since this option doesn't seem necessary then?

Moving to #4491 since I can't modify this PR