Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Fixed lack of es6 Object.assign in target envs like IE11 #391

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented May 26, 2017

Fixes 'regression' introduced in - #346 , which also should result in some minor perf/memory gain as spec gets shared on the prototype now and this needs to be done only once.

cc @istarkov

@istarkov
Copy link
Contributor

Seems like we can return back previous version, as now it's known that create-react-class supports older react versions.

@istarkov
Copy link
Contributor

(known - means I read it somewhere in twitter ;-))

@Andarist
Copy link
Contributor Author

Not sure we should return back to the create-react-class - its just another dependency, which is not that useful. Note in the docs about which package range support which React version should be quite enough.

Other thing is what browsers do we want to support out of the box (without need of polyfills to be provided etc)

@pebie
Copy link

pebie commented Jun 7, 2017

👍

@istarkov
Copy link
Contributor

istarkov commented Jun 7, 2017

There are linting errors in this PR, I can't accept it until it will be fixed.

@codecov-io
Copy link

Codecov Report

Merging #391 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   88.82%   88.85%   +0.02%     
==========================================
  Files          50       50              
  Lines         376      377       +1     
==========================================
+ Hits          334      335       +1     
  Misses         42       42
Impacted Files Coverage Δ
src/packages/recompose/lifecycle.js 81.81% <100%> (+1.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a74d7c0...277ec6b. Read the comment docs.

@Andarist
Copy link
Contributor Author

Andarist commented Jun 7, 2017

@istarkov fixed, had somehow misconfigured my editor and it didnt highlight the errors in the project

@istarkov
Copy link
Contributor

istarkov commented Jun 7, 2017

There were a bug in previous fix I did,
React.createClass has autobinding behaviour for all methods except lifecycle and methods in argument object prototype.

Don't you think we need this here?

@Andarist
Copy link
Contributor Author

Andarist commented Jun 7, 2017

The methods are put on the prototype so the this should be referenced correctly once the components get instantiated

@istarkov
Copy link
Contributor

istarkov commented Jun 7, 2017

How you will explain this if proto members referenced correctly?

class MClass {}; 
MClass.prototype.hello = function() { console.log(this);}; 
var a = new MClass(); 
a.hello(); // output undefined

@istarkov
Copy link
Contributor

istarkov commented Jun 7, 2017

Ooups I was wrong sorry ;-)

@istarkov istarkov merged commit d1f1b0c into acdlite:master Jun 7, 2017
@istarkov
Copy link
Contributor

istarkov commented Jun 7, 2017

Thank you!!!

@Andarist Andarist deleted the lack-of-assign-fix branch June 7, 2017 17:47
@istarkov
Copy link
Contributor

istarkov commented Jun 7, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants