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

Remove static properties for ie8 compatibility #2540

Merged
merged 1 commit into from
Nov 17, 2015

Conversation

mattydoincode
Copy link
Contributor

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
Copy link
Contributor

taion commented Nov 12, 2015

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

@mattydoincode
Copy link
Contributor Author

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
Copy link
Contributor

taion commented Nov 12, 2015

{
  "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
Copy link
Contributor Author

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

@taion
Copy link
Contributor

taion commented Nov 12, 2015

Can you squash those commits?

@mattydoincode
Copy link
Contributor Author

done

@taion
Copy link
Contributor

taion commented Nov 13, 2015

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

@mattydoincode
Copy link
Contributor Author

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

@taion
Copy link
Contributor

taion commented Nov 13, 2015

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@timdorr
Copy link
Member

timdorr commented Nov 13, 2015

Babel doesn't transform statics to normal object properties?

@taion
Copy link
Contributor

taion commented Nov 13, 2015

Nope, it uses Object.defineProperty.

@mattydoincode
Copy link
Contributor Author

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
Copy link
Contributor

taion commented Nov 13, 2015

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

@mattydoincode
Copy link
Contributor Author

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?

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

taion commented Nov 13, 2015

@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
Copy link
Member

timdorr commented Nov 13, 2015 via email

@mattydoincode
Copy link
Contributor Author

Woot! Thanks for everyone's help!

@timdorr
Copy link
Member

timdorr commented Nov 17, 2015

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

@taion
Copy link
Contributor

taion commented Nov 17, 2015

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

I'll merge though :D

taion added a commit that referenced this pull request Nov 17, 2015
Remove static properties for ie8 compatibility
@taion taion merged commit 22635f1 into remix-run:master Nov 17, 2015
@taion
Copy link
Contributor

taion commented Nov 17, 2015

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
Copy link
Contributor

taion commented Nov 25, 2015

@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
Copy link
Member

timdorr commented Nov 25, 2015

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

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
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.

3 participants