bublejs / buble

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

Non string keys are not excluded from rest object #149

Closed exe-dealer closed 5 years ago

exe-dealer commented 6 years ago
const source = { 1: 'one', 2: 'two' };
const { 1: _, ...rest } = source;
console.assert(!(1 in rest));

this test is failed when compiled with buble

function objectWithoutProperties (obj, exclude) { var target = {}; for (var k in obj) if (Object.prototype.hasOwnProperty.call(obj, k) && exclude.indexOf(k) === -1) target[k] = obj[k]; return target; }
var source = { 1: 'one', 2: 'two' };
var _ = source[1];
var rest$1 = objectWithoutProperties( source, [1] );
var rest = rest$1;
console.assert(!(1 in rest));

it uses indexOf which does strict equality so non-string keys are not ommited

adrianheine commented 5 years ago

Thanks for the report! This could be fixed during compile-time by writing strings into the exclude array, right?

function objectWithoutProperties (obj, exclude) { var target = {}; for (var k in obj) if (Object.prototype.hasOwnProperty.call(obj, k) && exclude.indexOf(k) === -1) target[k] = obj[k]; return target; }
var source = { 1: 'one', 2: 'two' };
var _ = source[1];
var rest$1 = objectWithoutProperties( source, ['1'] );
var rest = rest$1;
console.assert(!(1 in rest));
exe-dealer commented 5 years ago

Thanks for the report! This could be fixed during compile-time by writing strings into the exclude array, right?

function objectWithoutProperties (obj, exclude) { var target = {}; for (var k in obj) if (Object.prototype.hasOwnProperty.call(obj, k) && exclude.indexOf(k) === -1) target[k] = obj[k]; return target; }
var source = { 1: 'one', 2: 'two' };
var _ = source[1];
var rest$1 = objectWithoutProperties( source, ['1'] );
var rest = rest$1;
console.assert(!(1 in rest));

if you are asking me I think yes, 'writing strings' solution will cover all cases

adrianheine commented 5 years ago

Cool! So, currently, the code looks like this:

https://github.com/Rich-Harris/buble/blob/29eb1271e84a62a5b754a32252d3298759198893/src/utils/destructure.js#L141-L147

That basically means that all keys that are Identifiers and not computed will be quoted. This just has to be extended to cover keys that are Literals with a numeric value. Do you want to try that?

exe-dealer commented 5 years ago

its not enough just to wrap numeric literal in quotes because of following cases

const obj = { '0.1': 'foo', '100': 'bar' };
const { .1: a, 1e2: b, ...rest  } = obj;

This conversion can be at compile time but this requires much more code and time, so i'm suggesting pr with runtime fix. I thing this is enough rare case to not affect overall runtime perfomance. Also my pr fixes non-string computed keys like

const { [1+1]: foo, ...rest } = obj;
exe-dealer commented 5 years ago

hmm.. I forgot about case that user can redeclare String identifier

adrianheine commented 5 years ago

No, actually, if you use the value field of a numeric Literal key, you get a number, not the raw string. So with const { .1: a, 1e2: b, ...rest } = obj;, the value properties should be the numbers 0.1 and 100.

adrianheine commented 5 years ago

But you're right about non-string computed keys!

adrianheine commented 5 years ago

I don't think I want to consider the case that people redeclare String (or Object). That's going to break a lot of things anyway.

adrianheine commented 5 years ago

btw, I opened tc39/test262#2089.

exe-dealer commented 5 years ago

No, actually, if you use the value field of a numeric Literal key, you get a number, not the raw string. So with const { .1: a, 1e2: b, ...rest } = obj;, the value properties should be the numbers 0.1 and 100.

I updated pr