Fixed $$observable copying when spreading store (i.e. in applyMiddleware) by updating to babel@7-beta #2926
Conversation
…are) by updating to babel@7-beta
Andarist
force-pushed
the
fix/symbol-copying
branch
from
a61812c
to
facc060
Compare
|
Henry had come in a few months ago to try the Babel upgrade as well. I don't know what the timeframe is for the final release, but I'd like to wait on that. Being in beta means they can still break things, so I'd like to avoid a bad build being a possibility. We can keep this open until that happens (hopefully not too much longer!). |
|
@timdorr if this build passes there should be no possibility of breakage - all versions in pkg.json are locked. Also babel's upcoming v7 is not a rewrite, but rather an evolution of babel@6 - old test suite is still there, we only add to it (or ofc change in case of breaking changes) - so breakage possibility is also minimal on this front. I can sit on this, and I can even offer sending a second "concurring" PR for this to fix mentioned "bug", but it would literally be duplicating what's already done in babel@7, smth like this. Imho if an issue gets reported and fixed in a PR it would be good to merge it in - in either form. Would be a shame waiting with a fix until babel release "stable" version. But if you feel differently, it's ok - I don't mind, just commenting how I feel 😉 . Cheers! |
| @@ -61,14 +61,16 @@ | |||
| "symbol-observable": "^1.2.0" | |||
| }, | |||
| "devDependencies": { | |||
| "babel-cli": "^6.26.0", | |||
| "babel-core": "^6.26.0", | |||
| "@babel/core": "7.0.0-beta.44", | |||
There was a problem hiding this comment.
@babel/core and babel-core dependencies? Is it necessary to have both dependencies?
There was a problem hiding this comment.
This helps other tools to use peerDep with babel 6 or 7, in this case such tool is jest. You can read more about the issue here
| @@ -81,7 +83,7 @@ | |||
| "prettier": "^1.11.1", | |||
| "rimraf": "^2.6.2", | |||
| "rollup": "^0.57.1", | |||
| "rollup-plugin-babel": "^3.0.3", | |||
| "rollup-plugin-babel": "4.0.0-beta.4", | |||
There was a problem hiding this comment.
This plugin has released version 4.
"rollup-plugin-babel": "^4.0.1",
reduxexpose interop with stream libraries by exposing specialsymbol-observableon the store.However it gets lost in environments that do not polyfill
Object.assignor does it but not in a spec compliant way.How spreading differs from
Object.assign?Latter does not concern us, but first one is important in case of
$$observable.In non-es6+ envs
babelHelpers.extends(used by spread operator in babel@6 and in babel@7loosemode) fallback to a simple for-in iteration which does not iterate over symbols.However babel@7 has more spec-compliant version of this helper (
babelHelpers.objectSpread) which accounts for symbols.Side note
Sizes minified && gzipped
before this PR - 2386
after babel upgrade - 2281 (💰)
after switching to spec-compliant spread - 2347 (😢 , but still slightly better than before this PR)