Skip to content
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

Be able to use string type index in array, fix #5884 #5889

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

Peter-WF
Copy link
Contributor

@Peter-WF Peter-WF commented Jun 15, 2017

#5884

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@Peter-WF Peter-WF changed the title fix: Be able to use string type index in array, #5884 Be able to use string type index in array, fix #5884 Jun 15, 2017
@gebilaoxiong gebilaoxiong requested review from Hanks10100 and removed request for Hanks10100 June 15, 2017 11:43
@@ -189,7 +189,7 @@ export function defineReactive (
* already exist.
*/
export function set (target: Array<any> | Object, key: any, val: any): any {
if (Array.isArray(target) && typeof key === 'number') {
if (Array.isArray(target) && !isNaN(parseFloat(key))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like a double typeof... (+ key !== '') or key !== '' && typeof Number(key) === 'number' would be better. parseFloat looks like an overkill and isNaN is not supported on IE. wdyt @defcc ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNaN is supported in IE. The problem is that parseFloat would turn strings like "123ABC" into 123, which should probably be treated as string keys instead.

I think we should only support strings that are integers, something like typeof key === 'number' || /^\d+$/.test(key)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, isNaN exists as a global function but Number.isNaN does not exist 😨

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all!
It'd be a good idea to replace parseFloat with regular

@Peter-WF Peter-WF force-pushed the opt-global-api-set branch from 990c361 to 2693b78 Compare June 15, 2017 15:36
@Peter-WF Peter-WF force-pushed the opt-global-api-set branch from 2693b78 to 0231b68 Compare June 15, 2017 15:39
@yyx990803 yyx990803 merged commit 8a2c514 into vuejs:dev Jun 16, 2017
awamwang added a commit to awamwang/vue that referenced this pull request Jun 28, 2017
* 'dev' of https://github.com/vuejs/vue:
  build: use cross-platform hook installation with shelljs
  build: move test config files into /test
  build: add script for generating release note
  build: add git commit message validation
  fix(v-model): use consistent behavior during IME composition for other text-like input types (fix vuejs#5902)
  simplify source with rest params
  fix slot resolved incorrect with abstract component (fix vuejs#5888) (vuejs#5895)
  test:improve reserved props test
  test:add bind object test
  fix:when using object syntax in v-bind, special attribute have no effect
  Be able to use string type index in array (vuejs#5889)
  Merge inject when extending a component (vuejs#5827)

# Conflicts:
#	.gitignore
#	src/core/util/options.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants