Update for react 15.5 by zackify · Pull Request #497 · text-mask/text-mask

zackify · 2017-04-10T14:29:49Z

Even better would be to move away from react.createClass. But this will make the component work in React 15.5 and 16.

bbenezech · 2017-04-14T10:14:21Z

@zackify Can you fix linting complaints to green the build? It looks like it is a no dangling comma, no semi, no curly-space repo. ;)
@lozjackson can you merge this? React "warnings" are console errors, are very noisy and fail CIs.

zackify · 2017-04-14T10:56:34Z

What do you think about switching the linting to prettier? A lot of big JS projects are doing it and it removes the need to waste time talking about formatting. My editor runs it on save.

Also, if someone more familiar with how this pulls in the base text mask part could help, I would like to switch it from create class so that you don't need that little extra library. Thanks:)

lozjackson · 2017-04-14T12:04:33Z

I've been busy lately.. I'll look into this when I get a minute.

about the linting.. I'd prefer to leave it as is for now.

Also, if someone more familiar with how this pulls in the base text mask part could help

text-mask-core is imported into the react component like this

import createTextMaskInputElement from '../../core/src/createTextMaskInputElement'

The angular2-text-mask and ember-text-mask components both import the text-mask-core package from npm as a dependency.. personally I prefer this approach.

I'm not sure why the other components were built the way they were (they were built before my involvement with text mask)... so I'm not really sure of all the pros and cons to each method.

I would like to switch it from create class so that you don't need that little extra library

could you show an example? what changes would be required for this?

lozjackson · 2017-04-14T12:12:21Z

@browniefed @mzedeler do you have any thoughts on this

browniefed · 2017-04-14T15:27:28Z

Can we just switch to using Component? No reason to bring in create-class

zackify · 2017-04-14T15:46:34Z

I just figured it out and removed createClass. Let me know if you want me to get rid of prettier, if someone else could update the commit to do that and conform to your styles, go for it. Otherwise if you are interested, I could add it on precommit for you guys :)

P.S. Have you guys thought about removing the build code from git?

I can understand the frustration of prettier being ran when I save files btw, if you want a new pr with just my changes I will turn it off and branch off again.

lozjackson · 2017-04-14T15:53:43Z

-      <form className='form-horizontal'>
-        <div className='form-group'>
-          <label htmlFor='1' className='col-sm-2 control-label'>Masked input</label>
+export default () => (


does the example app need these changes?

Are the changes this PR makes backwards compatible? will existing users of the react component have to make any changes to their app/implementation for this to work?

The change is needed to get rid of createClass which is deprecated. Functional components have been around since react 14. This should all be fully backwards compatible. I can run the tests with React 14 and see if you'd like?

lozjackson · 2017-04-14T15:57:46Z

sorry.. can you limit your changes to only change things that are required to fix or implement your PR.. with so many changes it is difficult to see what has actually changed that matters.. and is gonna take a lot longer to test/merge this.

I don't mind fixing a few linting issues before merging, but my time is limited

zackify · 2017-04-14T16:00:25Z

@lozjackson I should have them fixed, I just need to run the tests through the lint command you have and it should be all set. I only changed what was neccesary minus prettier that was auto linting. Sorry about that!

lozjackson · 2017-04-14T16:10:05Z

are any of the changes made to the tests required for your PR?

lozjackson · 2017-04-14T16:16:34Z

      <input
        {...props}
-        onInput={this.onChange}
+        onInput={event => this.onChange(event)}


is this change required?

Yes, createClass autobinds methods on the components, classes do not. So you have to use an arrow function to keep the value of this

oh, ok fair enough

It shouldn't be needed. As I understand it, the old version may perform a little better.

@mzedeler it most definitely is needed, otherwise the onChange function will not have this

@zackify Why not put the arrow on the function declaration or bind in constructor?
I see no reason to instantiate a new function at each render.

Sure I can add it to the constructor. I just don't see it as a big deal since arrow functions are already more performant in node than bound ones nodejs/node#3622

Let me know if you want me to do that for sure! 😎

lozjackson · 2017-04-14T16:16:56Z

+        onInput={event => this.onChange(event)}
        defaultValue={this.props.value}
-        ref={(inputElement) => (this.inputElement = inputElement)}
+        ref={inputElement => this.inputElement = inputElement} //eslint-disable-line


is this change required?

The linter complains about assignment to an arrow function, so i disabled the line

but the commas surrounding it do the same thing - does it need changing?

Without that line I get this.

It should be ref={inputElement => { this.inputElement = inputElement }} or better, a bound function handle (same as onChange).

lozjackson · 2017-04-14T16:19:10Z

-})
+}
+
+MaskedInput.propTypes = {


why move this code block to the bottom? can this be defined on the class like it was? ..can it be put back where it was? or is there a valid reason for moving it

proptypes are static methods and have to be added to classes like this unless if you want to add static prop types which isn't a standard in js yet

lozjackson · 2017-04-14T16:21:27Z

-    showMask: PropTypes.bool
-  },
-
-  createTextMaskInputElement,


why remove this?

You don't need to add createTextMaskInputElement to the component, it just gets called in initTextMask

no it is not needed, true.. however, it was put there for a reason and I don't see the need to remove it

Because you can't just pass the value into a component class. You would be adding it to the instance, like "this.createttextmask = createTextMask" and then calling it on the instance. It's pointless. There was not a reason it needed to be there in the first place. It could have always been called directly

ok.. there is a reason for it being there though.. it is to aid testing - so that you can test the function exists and is a function.. i agree it is a little pointless and the component works just the same without it.. I'm unsure if I agree with removing it.. it is up for debate..

It tests that the createTextMaskInputElement function is imported correctly into the react component. I can see that it is "testing for the sake of testing", and perhaps not needed...

also.. this test and this test are relying on being able to stub the createTextMaskInputElement method - currently they are not failing because the method is not being called on the component class.. again, it is not needed to make the component function.. but is there to aid testing.

It might be that there is a better way to test this.. (ideas/PRs welcome).. but I think this particular PR should not make those changes if it doesn't need to.

I see, I will make an adjustment to support these test cases for you!

FYI, tests shouldn't be allowed to successfully skip, I'll fix that as well.

I just made a new commit that makes the component itself a function that expects createTextMaskInputElement to be passed into it, this way you can pass in a mock easily in tests. All tests pass.

lozjackson · 2017-04-14T16:26:46Z

I'm not trying to be awkward.. its just that you've made a lot of changes to things that don't need to be changed.. and if a bug is introduced as a result of this PR, then it will make it more difficult to track down the exact changes.

If we want to make changes to code formatting, linting or other things then that should be done in a separate PR that targets just those things.. This PR should be focussed and targeted at only making changes that are required to fix the specific issue #496

zackify · 2017-04-14T16:31:32Z

I didn't make changes that were unnecessary and explained in each of your comments :) I fixed the test linting right now for you.

zackify · 2017-04-14T16:35:23Z

-    )
-    expect(typeof maskedInput.createTextMaskInputElement).to.equal('function')
-  })
-


This isn't needed because createTextMaskInputElement is called directly, see the initTextMask function

lozjackson · 2017-04-14T16:37:33Z

-
-  createTextMaskInputElement,
+import React from 'react'
+import PropTypes from 'prop-types'


does PropTypes need to be imported separately? whats the advantage? .. or is the previous method of importing PropTypes not supported any longer?

Yes. it's deprecated in React 15.5 and you have to import it from a separate package

lozjackson · 2017-04-14T19:12:32Z

@zackify thanks for tidying this up.. its much easier to see the relevant changes.. We should be in a better position to merge this now.. Unfortunately I don't have time to do that right now.. hopefully will do later today, or over the weekend.

zackify · 2017-04-14T19:51:52Z

@lozjackson thanks, sounds good!

lozjackson · 2017-04-14T20:26:25Z

@zackify sorry.. I appreciate your efforts but can you revert that last commit., and then make those changes in a new PR so I can review the changes separately.. keep this PR focused.

zackify · 2017-04-14T20:27:33Z

Sure, but i can't really make that pr without the new version. I'll make two, one with and without the commit

Update for react 15.5

d809cfb

Remove create class

84b703d

Remove pointless test, fixed onChange

3c27769

lozjackson reviewed Apr 14, 2017

View reviewed changes

Run linter

a0ee7ac

lozjackson reviewed Apr 14, 2017

View reviewed changes

Fix linting

cf2ca05

Fix example app formatting

55740ae

zackify commented Apr 14, 2017

View reviewed changes

lozjackson reviewed Apr 14, 2017

View reviewed changes

Fix linter error on ref

11a71fc

zackify force-pushed the master branch from 7e0b8a3 to 11a71fc Compare April 14, 2017 20:29

lozjackson merged commit 3618741 into text-mask:master Apr 15, 2017

lozjackson mentioned this pull request Apr 24, 2017

Console warning "Failed prop type: React.PropTypes type checking code is stripped in production." when using react-text-mask #510

Closed

Conversation

zackify commented Apr 10, 2017

Uh oh!

bbenezech commented Apr 14, 2017

Uh oh!

zackify commented Apr 14, 2017

Uh oh!

lozjackson commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lozjackson commented Apr 14, 2017

Uh oh!

browniefed commented Apr 14, 2017

Uh oh!

zackify commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lozjackson commented Apr 14, 2017

Uh oh!

zackify commented Apr 14, 2017

Uh oh!

lozjackson commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lozjackson Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lozjackson commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lozjackson commented Apr 14, 2017 •

edited

Loading

zackify commented Apr 14, 2017 •

edited

Loading

lozjackson Apr 14, 2017 •

edited

Loading

lozjackson commented Apr 14, 2017 •

edited

Loading

lozjackson Apr 14, 2017 •

edited

Loading

zackify commented Apr 14, 2017 via email •

edited

Loading