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

object-rest not behaving as expected with computed properties #6341

Open
lslv1243 opened this Issue Sep 29, 2017 · 11 comments

Comments

Projects
None yet
8 participants
@lslv1243

lslv1243 commented Sep 29, 2017

Choose one: is this a bug report or feature request?
bug

Input Code

const a = { 1: 0, 2: 1, 3: 2 }
const i = 1
const { [i]: val, ...rest } = a

Babel Configuration (.babelrc, package.json, cli command)

{
  "presets": ["babel-preset-expo"],
  "env": {
    "development": {
      "plugins": ["transform-react-jsx-source"]
    }
  }
}

Expected Behavior

rest should be

{
  "2": 1,
  "3": 2,
}

Current Behavior

rest is

{
  "1": 0,
  "2": 1,
  "3": 2,
}

Possible Solution

indexOf compares with 3 equals and the object key is represented as string

if (excluded.indexOf(key) >= 0) continue;

Context

running react-native application

Your Environment

using react native with expo

{
  "expo": {
    "sdkVersion": "21.0.0"
  }
}

dont know about configuration

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Sep 29, 2017

Collaborator

Hey @lslv1243! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Sep 29, 2017

Hey @lslv1243! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 29, 2017

Member

Thanks for the report @lslv1243 . It is indeed a bug, let me look into fixing it.

Member

Andarist commented Sep 29, 2017

Thanks for the report @lslv1243 . It is indeed a bug, let me look into fixing it.

@Andarist Andarist self-assigned this Sep 29, 2017

@Andarist Andarist added the i: bug label Sep 29, 2017

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 29, 2017

Member

This already got fixed by #5757 and the fix is available in v7.

For some reason when having preset-es2015 + stage-3 active in the repl it creates buggy output. stage-3 alone is working as expected.

Member

Andarist commented Sep 29, 2017

This already got fixed by #5757 and the fix is available in v7.

For some reason when having preset-es2015 + stage-3 active in the repl it creates buggy output. stage-3 alone is working as expected.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 29, 2017

Member

Ok, got down to it - in v7 there is still a conflict when using both:

  • babel-plugin-transform-es2015-destructuring
  • babel-plugin-transform-object-rest-spread

Actual output

var a = {
  1: 0,
  2: 1,
  3: 2
};
var i = 1;
var val = a[i],
    rest = babelHelpers.objectWithoutProperties(a, [i]);

Expected output

var a = {
  1: 0,
  2: 1,
  3: 2
};
var i = 1;
var val = a[i],
    rest = babelHelpers.objectWithoutProperties(a, [i].map(babelHelpers.toPropertyKey));
Member

Andarist commented Sep 29, 2017

Ok, got down to it - in v7 there is still a conflict when using both:

  • babel-plugin-transform-es2015-destructuring
  • babel-plugin-transform-object-rest-spread

Actual output

var a = {
  1: 0,
  2: 1,
  3: 2
};
var i = 1;
var val = a[i],
    rest = babelHelpers.objectWithoutProperties(a, [i]);

Expected output

var a = {
  1: 0,
  2: 1,
  3: 2
};
var i = 1;
var val = a[i],
    rest = babelHelpers.objectWithoutProperties(a, [i].map(babelHelpers.toPropertyKey));
@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Sep 29, 2017

Member

There's all kinds of bugs with using destructuring and object-rest-spread together. It's on my list of todos.

Member

jridgewell commented Sep 29, 2017

There's all kinds of bugs with using destructuring and object-rest-spread together. It's on my list of todos.

@cgood92

This comment has been minimized.

Show comment
Hide comment
@cgood92

cgood92 Oct 2, 2017

Having the same, or at least nearly the same, problem.

const obj = {
  0: true,
  1: 'hi',
  2: true,
}

const { [1]: number1, ...rest1 } = obj
const { ['1']: number2, ...rest2 } = obj


console.log(number1, rest1)
// => "hi", { 0: true, 1: 'hi', 2: true }

console.log(number2, rest2)
// => "hi", { 0: true, 2: true }

cgood92 commented Oct 2, 2017

Having the same, or at least nearly the same, problem.

const obj = {
  0: true,
  1: 'hi',
  2: true,
}

const { [1]: number1, ...rest1 } = obj
const { ['1']: number2, ...rest2 } = obj


console.log(number1, rest1)
// => "hi", { 0: true, 1: 'hi', 2: true }

console.log(number2, rest2)
// => "hi", { 0: true, 2: true }

@Andarist Andarist removed their assignment Oct 21, 2017

@loganfsmyth loganfsmyth changed the title from rest operator not behaving as expected to object-rest not behaving as expected with computed properties Oct 23, 2017

@PJCHENder

This comment has been minimized.

Show comment
Hide comment
@PJCHENder

PJCHENder Sep 18, 2018

@Andarist

It seems that this bug has not been fixed yet in Babel v7.
I use Babel REPL to reproduce the issue.
https://goo.gl/7ed5Tq

The compiled version still not using String as key:

let obj = { 1: 1, 2: 2, 3: 3, 4: 4 };
const { 1: value } = obj,
  newState = _objectWithoutProperties(obj, [1]);

To get the expected result, it should be:

const { 1: value } = obj,
  newState = _objectWithoutProperties(obj, ['1']);

Therefore, the rest property still won't work correctly when the object key is Number.

PJCHENder commented Sep 18, 2018

@Andarist

It seems that this bug has not been fixed yet in Babel v7.
I use Babel REPL to reproduce the issue.
https://goo.gl/7ed5Tq

The compiled version still not using String as key:

let obj = { 1: 1, 2: 2, 3: 3, 4: 4 };
const { 1: value } = obj,
  newState = _objectWithoutProperties(obj, [1]);

To get the expected result, it should be:

const { 1: value } = obj,
  newState = _objectWithoutProperties(obj, ['1']);

Therefore, the rest property still won't work correctly when the object key is Number.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 18, 2018

Member

There is indeed some problem, at least with the REPL. It's weird because REPL shows different outputs than what we have as expected results in our tests (tests are handling this correctly - by introducing a toPropertyKey helper)

Could you test this in a sample project?

Member

Andarist commented Sep 18, 2018

There is indeed some problem, at least with the REPL. It's weird because REPL shows different outputs than what we have as expected results in our tests (tests are handling this correctly - by introducing a toPropertyKey helper)

Could you test this in a sample project?

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Sep 18, 2018

Member

The repl still uses Babel 6 by default. You can use https://babeljs.io/repl/build/master to check the current behavior.

Member

nicolo-ribaudo commented Sep 18, 2018

The repl still uses Babel 6 by default. You can use https://babeljs.io/repl/build/master to check the current behavior.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 18, 2018

Member

And this newer REPL also shows different results than tests on master 🤔 I have also previously tested this on the older REPL but I have used it with babel@7 by changing the query params (although couldnt use latest for some reason there, so I was stuck on rc4 or some similar version)

Member

Andarist commented Sep 18, 2018

And this newer REPL also shows different results than tests on master 🤔 I have also previously tested this on the older REPL but I have used it with babel@7 by changing the query params (although couldnt use latest for some reason there, so I was stuck on rc4 or some similar version)

@PJCHENder

This comment has been minimized.

Show comment
Hide comment
@PJCHENder

PJCHENder Sep 19, 2018

Thanks for testing, @Andarist.
I found this issue with the environment settings: ["@babel/core": "^7.0.1", "@babel/preset-env"] which was based on a React project.

PJCHENder commented Sep 19, 2018

Thanks for testing, @Andarist.
I found this issue with the environment settings: ["@babel/core": "^7.0.1", "@babel/preset-env"] which was based on a React project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment