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

fix(core): Use native bind function instead of own (fix #7408) #7491

Merged
merged 5 commits into from
Mar 8, 2018
Merged

fix(core): Use native bind function instead of own (fix #7408) #7491

merged 5 commits into from
Mar 8, 2018

Conversation

dima-takoy-zz
Copy link
Contributor

Simple refactor bind function for easy revert.
Custom bind implementation performance is lower than native. ( https://jsperf.com/vue-bind-perf )

issue link: #7408

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:

Simple refactor bind function for easy revert.
Custom bind implementation performance is lower than native.

https://jsperf.com/vue-bind-perf
@dima-takoy-zz dima-takoy-zz changed the title fix(core): Use native bind instead of own fix #7408 fix(core): Use native bind function instead of own. fix #7408 Jan 20, 2018
@dima-takoy-zz dima-takoy-zz changed the title fix(core): Use native bind function instead of own. fix #7408 fix(core): Use native bind function instead of own. (fix #7408) Jan 20, 2018
@dima-takoy-zz dima-takoy-zz changed the title fix(core): Use native bind function instead of own. (fix #7408) fix(core): Use native bind function instead of own (fix #7408) Jan 20, 2018
@yyx990803
Copy link
Member

Some browsers in the support range e.g. PhantomJS don't have Function.prototype.bind, so we should only use the native version when it exists.

@OEvgeny
Copy link

OEvgeny commented Jan 22, 2018

Maybe it's better to decide at first module run which implementation to use? e.g.:

export const bind = Function.prototype.bind ? nativeBind : bind

@dima-takoy-zz
Copy link
Contributor Author

Maybe it's better to decide at first module run which implementation to use? e.g.:

const ownBind = (fn: Function, ctx: Object) => {
  function boundFn(a) {
    const l = arguments.length
    return l
      ? l > 1
        ? fn.apply(ctx, arguments)
        : fn.call(ctx, a)
      : fn.call(ctx)
  }

  boundFn._length = fn.length
  return boundFn
}

export const bind = Function.prototype.bind ? Function.prototype.bind : ownBind

@dima-takoy-zz
Copy link
Contributor Author

Some browsers in the support range e.g. PhantomJS don't have Function.prototype.bind, so we should only use the native version when it exists.

Why we should carry polyfill for this? Phantomjs is a virtual browser that can be used for tests and scrapping. We carry polyfill for case when any vuejs site can be scrapped?

We increase bundle size for every build for ^ case?

I just didn't understood.

@OEvgeny
Copy link

OEvgeny commented Jan 22, 2018

I think

...
export const bind = Function.prototype.bind ? Function.prototype.bind : ownBind

Should be:

function ownBind(fn: function, ctx: object) {
  // internal/current implementation ...
}

function nativeBind(fn: function, ctx: object) {
  return fn.bind(ctx)
}

export const bind = Function.prototype.bind ? nativeBind : ownBind

Because of how native bind should be called.

@dima-takoy-zz
Copy link
Contributor Author

So maybe wrap polyfill version in if (process.NODE_ENV == 'development') ? its removes dead code in production builds and broke dead some e2e tests and scrappers.

I don't see its as as breaking change.

@yyx990803
Copy link
Member

It is a breaking change if you remove a polyfill.

Someone's tests that were able to run in PhantomJS would break suddenly in a patch release because of that.

@dima-takoy-zz
Copy link
Contributor Author

dima-takoy-zz commented Jan 22, 2018

It is a breaking change if you remove a polyfill.

Then its ready for merge.

@yyx990803
Copy link
Member

I think @OEvgeny 's suggestion should be applied so that we only check native bind once on initialization.

boundFn._length = fn.length
return boundFn
}

function nativeBind(fn: Function, ctx: Object): Function {
return fn.bind(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the optional arguments, cause different behavior to ownBind

Copy link

Choose a reason for hiding this comment

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

I see no optional arguments in ownBind. Are you about fn._length? If yes. It shouldn't be a problem because we should check both length and _length since we don't know exactly was the fn bound via our ownBind or not. e.g.:

(enterHook._length || enterHook.length) > 1

@yyx990803 yyx990803 merged commit dc2171a into vuejs:dev Mar 8, 2018
@rlightner
Copy link

I think this broke something. if fn.bind is undefined it blows up.

@yyx990803
Copy link
Member

@rlightner please provide a reproduction. fn.bind is only used when Function.prototype.bind exists. fn.bind cannot be undefined in that case unless fn is not a function.

@rlightner
Copy link

rlightner commented Mar 11, 2018

EDIT: I think this issue exposed a coding error. This should be a computed and not a method. It worked somehow before this fix though.

I'll try. The fn being passed in is a method in a template that contains:

drawer: {
    get() {
        return this.$store.state.drawer
    }
    , set(value) {
        this.$store.commit('toggleDrawer', value)
    }
}

and it's being passed in to nativeBind as the get/set. That doesn't have a bind(). This is in a normal vue template.

image

@rjoo
Copy link

rjoo commented Apr 9, 2018

As @rlightner pointed out, this exposed a coding error if whatever's provided to the bind helper is not a function.

Version 2.5.13 https://jsfiddle.net/ujxu9mvd/ Renders fine, fails silently
Version 2.5.14 https://jsfiddle.net/bad4a8mb/1/ Does NOT render and it throws an Uncaught TypeError

f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
aJean pushed a commit to aJean/vue that referenced this pull request Aug 19, 2020
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.

7 participants