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

do not pass i13n props to string components #96

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

sebastiendavid
Copy link
Contributor

To avoid to pass i13n props to string components such a or button, we can filter the props before passing them to the wrapped component. So React ^15.2.0 will not warn about unknown props. What do you think?

@yahoocla
Copy link

yahoocla commented Aug 2, 2016

CLA is valid!

@kaesonho
Copy link
Contributor

kaesonho commented Aug 2, 2016

👍 thanks @sebastiendavid for fixing this, overall the strategy is good, not sure why build failed, will check and get this merge soon

@sebastiendavid
Copy link
Contributor Author

Yes I don't understand which step is failing during the tests. For me grunt cover works fine locally.

@roderickhsiao
Copy link
Collaborator

👍

@kaesonho
Copy link
Contributor

kaesonho commented Aug 2, 2016

I just check out the repo and ran it locally by grunt unit

I can see this test error

  1) createI13nNode should not pass i13n props to string components:
     Error: expected undefined to sort of equal { href: '#/foobar', children: undefined }
      at Assertion.assert (node_modules/expect.js/index.js:96:13)
      at Assertion.eql (node_modules/expect.js/index.js:230:10)
      at Context.<anonymous> (tmp/tests/unit/utils/createI13nNode.js:452:61)
      at Object.done (tmp/tests/unit/utils/createI13nNode.js:92:13)
      at node_modules/jsdom/lib/jsdom.js:249:18

could you check? @sebastiendavid

I'm thinking that might be Array.find, as we are using node12 for ci, https://github.com/yahoo/react-i13n/blob/master/.travis.yml#L7

@sebastiendavid
Copy link
Contributor Author

Ah yes of course, I did not change my node version, I was using node v4. I will make it compatible for node v0.12 tomorrow, it's late here ;)

@kaesonho
Copy link
Contributor

kaesonho commented Aug 2, 2016

thank you @sebastiendavid ,
yeah we definitely need to update to support running ci on newer node version, here's the PR #97

but we will still running it on 0.12 since it's still being used

@kaesonho
Copy link
Contributor

kaesonho commented Aug 2, 2016

let me merge this since we already know it's due to node0.12 and we already update to run ci on node4/6 after #97

@kaesonho
Copy link
Contributor

kaesonho commented Aug 2, 2016

@sebastiendavid there's a node version update on master, could you just rebase master? I was going to merge this but thought it might be better to see func test is also passing for safety.

@sebastiendavid
Copy link
Contributor Author

done

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.1%) to 90.879% when pulling ab90092 on sebastiendavid:unknownprops into ae127ea on yahoo:master.

@kaesonho kaesonho merged commit a1fb377 into yahoo:master Aug 3, 2016
@kaesonho
Copy link
Contributor

kaesonho commented Aug 3, 2016

@sebastiendavid
Copy link
Contributor Author

Thanks :)

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

Successfully merging this pull request may close these issues.

6 participants