Remove static properties for ie8 compatibility #2540

mattydoincode · 2015-11-12T21:37:35Z

By removing usages of class-properties which are still experimental, ie8 is fully supported. We also protect against it happening in the future with a blacklist of that feature in babel.

taion · 2015-11-12T22:02:42Z

Please either bump .babelrc to stage 1 or blacklist the class properties transform so future changes can't break this.

mattydoincode · 2015-11-12T22:05:18Z

How do I blacklist a plugin for babel? Googling proved messy. I think if that's doable, it'd be best since it's the smallest surface area change.

taion · 2015-11-12T22:07:20Z

{
  "stage": 0,
  "loose": "all",
  "blacklist": [
    "es7.classProperties"
  ],
  "plugins": [
    "dev-expression"
  ]
}

off the top of my head, but you should double check that this works (i.e. that Babel throws a fit at you if you then try to use a class property)

mattydoincode · 2015-11-12T22:13:27Z

Boom. it worked! Well done. Some features are hard to find, didn't know that was a thing!

taion · 2015-11-12T22:14:11Z

Can you squash those commits?

mattydoincode · 2015-11-12T22:16:58Z

done

taion · 2015-11-13T15:34:23Z

You need to update the tests as well, I think.

mattydoincode · 2015-11-13T15:41:21Z

all the tests pass for me locally, are you saying there is a test I should add for this?

taion · 2015-11-13T15:44:34Z

Not with the Babel changes. See e.g. https://github.com/rackt/react-router/blob/v1.0.0/modules/__tests__/Router-test.js#L258-L260.

Might make more sense to de-blacklist that transform for tests, though.

timdorr · 2015-11-13T17:00:31Z

modules/IndexRedirect.js

+  onEnter: falsy,
+  children: falsy
+}
+IndexRedirect.createRouteFromReactElement = function (element, parentRoute) {


(nit) The spacing is off on this one. This is purely a style thing.

Sure, but what's off exactly? I'm adding a space between the propTypes and the function creation, is that what you're talkin about? Just want to make sure I understand.

Yes, that and the two spaces after the createRouteFromReactElement function. It looks like that's just shifted up one line.

timdorr · 2015-11-13T17:01:44Z

Babel doesn't transform statics to normal object properties?

taion · 2015-11-13T17:05:41Z

Nope, it uses Object.defineProperty.

mattydoincode · 2015-11-13T17:12:09Z

Hey guys,

Sorry I'm having issues figuring out how to best make the tests not use the
default babelrc but instead use a different one. The babel loader is always
a mystery to me in how much it respects the default babel parameters... Any
help would be greatly appreciated.

On Fri, Nov 13, 2015 at 12:05 PM, Jimmy Jia notifications@github.com
wrote:

Nope, it uses Object.defineProperty.

—
Reply to this email directly or view it on GitHub
#2540 (comment).

taion · 2015-11-13T18:49:27Z

Just clean up the tests to not use class properties then. Probably easiest way to do it.

mattydoincode · 2015-11-13T20:02:15Z

Hey guys, I've made the appropriate changes, can someone confirm that this (https://github.com/rackt/react-router/pull/2540/files#diff-82b958d0880900cdd7c05f94eb64e281R271) looks okay to them?

taion · 2015-11-13T20:07:53Z

modules/__tests__/Router-test.js

@@ -272,7 +268,7 @@ describe('Router', function () {
        render() {
          const { label, children } = this.props
          const child = React.cloneElement(children, {
-            createElement: this.createElement
+            createElement: this.createElement.bind(this)


Nit - please bind this in the constructor. Even though this is in test code, I'd rather not users accidentally follow this pattern without recognizing the consequences.

Ah, my bad. For my own knowledge, what is the downside of this? Just that
one would have to continually bind it vs it being bound for every use?

On Fri, Nov 13, 2015 at 3:08 PM, Jimmy Jia notifications@github.com wrote:

In modules/tests/Router-test.js
#2540 (comment):

@@ -272,7 +268,7 @@ describe('Router', function () {
render() {
const { label, children } = this.props
const child = React.cloneElement(children, {

createElement: this.createElement

createElement: this.createElement.bind(this)

Nit - please bind this in the constructor. Even though this is in test
code, I'd rather not users accidentally follow this pattern without
recognizing the consequences.

—
Reply to this email directly or view it on GitHub
https://github.com/rackt/react-router/pull/2540/files#r44828136.

Yes - it breaks referential equality, which leads to e.g. pure render not working. It seldom ever really matters, but it's just good hygiene.

Awesome, thanks for the explanation. And your help in general. (Happy
friday everyone!).

On Fri, Nov 13, 2015 at 3:24 PM, Jimmy Jia notifications@github.com wrote:

In modules/tests/Router-test.js
#2540 (comment):

@@ -272,7 +268,7 @@ describe('Router', function () {
render() {
const { label, children } = this.props
const child = React.cloneElement(children, {

createElement: this.createElement

createElement: this.createElement.bind(this)

Yes - it breaks referential equality, which leads to e.g. pure render not
working. It seldom ever really matters, but it's just good hygiene.

—
Reply to this email directly or view it on GitHub
https://github.com/rackt/react-router/pull/2540/files#r44829782.

taion · 2015-11-13T23:15:03Z

@timdorr Did this catch all the code nits you saw? I think dropping static properties is worth it if that's all it takes for us to support IE8.

timdorr · 2015-11-13T23:27:31Z

LGTM.

mattydoincode · 2015-11-16T23:27:27Z

Woot! Thanks for everyone's help!

timdorr · 2015-11-17T18:02:27Z

@taion Did you want to merge this or should I?

taion · 2015-11-17T18:11:01Z

I don't want @mjackson to yell at me again 🙈 😆

I'll merge though :D

Remove static properties for ie8 compatibility

taion · 2015-11-17T19:07:03Z

Oops, slipped up - https://github.com/rackt/react-router/pull/2540/files#diff-82b958d0880900cdd7c05f94eb64e281R259 is wrong. I'm just going to fix in a separate PR.

taion · 2015-11-25T21:03:18Z

@ryanflorence What do you think of cutting a v1.0.1 for this? Thus far, as far as I can tell, nothing else has come up that we can resolve before v1.1.0.

timdorr · 2015-11-25T21:08:19Z

I would love to see that too. It's been two weeks, so a minor update isn't a big deal 😄

mattydoincode force-pushed the master branch from 44787a5 to 7d7dc75 Compare November 12, 2015 22:16

timdorr reviewed Nov 13, 2015
View reviewed changes

mattydoincode force-pushed the master branch from c4ea78c to 6e07091 Compare November 13, 2015 20:02

taion reviewed Nov 13, 2015
View reviewed changes

protect ie8!

a0877f9

mattydoincode force-pushed the master branch from 6e07091 to a0877f9 Compare November 13, 2015 20:12

mattydoincode mentioned this pull request Nov 13, 2015

Protect IE8, disallow es7 class properties redux-form/redux-form#274

Closed

taion added a commit that referenced this pull request Nov 17, 2015

Merge pull request #2540 from mattydoincode/master

22635f1

Remove static properties for ie8 compatibility

taion merged commit 22635f1 into remix-run:master Nov 17, 2015

taion mentioned this pull request Nov 17, 2015

Fix using this before super #2563

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove static properties for ie8 compatibility #2540

Remove static properties for ie8 compatibility #2540

mattydoincode commented Nov 12, 2015

taion commented Nov 12, 2015

mattydoincode commented Nov 12, 2015

taion commented Nov 12, 2015

mattydoincode commented Nov 12, 2015

taion commented Nov 12, 2015

mattydoincode commented Nov 12, 2015

taion commented Nov 13, 2015

mattydoincode commented Nov 13, 2015

taion commented Nov 13, 2015

timdorr Nov 13, 2015

mattydoincode Nov 13, 2015

timdorr Nov 13, 2015

timdorr commented Nov 13, 2015

taion commented Nov 13, 2015

mattydoincode commented Nov 13, 2015

taion commented Nov 13, 2015

mattydoincode commented Nov 13, 2015

taion Nov 13, 2015

mattydoincode Nov 13, 2015

taion Nov 13, 2015

mattydoincode Nov 13, 2015

taion commented Nov 13, 2015

timdorr commented Nov 13, 2015 via email

mattydoincode commented Nov 16, 2015

timdorr commented Nov 17, 2015

taion commented Nov 17, 2015

taion commented Nov 17, 2015

taion commented Nov 25, 2015

timdorr commented Nov 25, 2015

Remove static properties for ie8 compatibility #2540

Remove static properties for ie8 compatibility #2540

Conversation

mattydoincode commented Nov 12, 2015

taion commented Nov 12, 2015

mattydoincode commented Nov 12, 2015

taion commented Nov 12, 2015

mattydoincode commented Nov 12, 2015

taion commented Nov 12, 2015

mattydoincode commented Nov 12, 2015

taion commented Nov 13, 2015

mattydoincode commented Nov 13, 2015

taion commented Nov 13, 2015

timdorr Nov 13, 2015

Choose a reason for hiding this comment

mattydoincode Nov 13, 2015

Choose a reason for hiding this comment

timdorr Nov 13, 2015

Choose a reason for hiding this comment

timdorr commented Nov 13, 2015

taion commented Nov 13, 2015

mattydoincode commented Nov 13, 2015

taion commented Nov 13, 2015

mattydoincode commented Nov 13, 2015

taion Nov 13, 2015

Choose a reason for hiding this comment

mattydoincode Nov 13, 2015

Choose a reason for hiding this comment

taion Nov 13, 2015

Choose a reason for hiding this comment

mattydoincode Nov 13, 2015

Choose a reason for hiding this comment

taion commented Nov 13, 2015

timdorr commented Nov 13, 2015 via email

mattydoincode commented Nov 16, 2015

timdorr commented Nov 17, 2015

taion commented Nov 17, 2015

taion commented Nov 17, 2015

taion commented Nov 25, 2015

timdorr commented Nov 25, 2015