Skip to content

Add TypeScript tests for the rename-unsafe-lifecycles transform#236

Merged
threepointone merged 2 commits intoreactjs:masterfrom
skovy:skovy/add-rename-lifecycle-typescript-test
Jul 27, 2019
Merged

Add TypeScript tests for the rename-unsafe-lifecycles transform#236
threepointone merged 2 commits intoreactjs:masterfrom
skovy:skovy/add-rename-lifecycle-typescript-test

Conversation

@skovy
Copy link
Contributor

@skovy skovy commented Jul 27, 2019

Only the tests from #228.

The transform has to be mocked because jscodeshift doesn't support testing other parsers/file extensions.

I named the examples class.tsx.input.js (must be js to match what jscodeshift expects) but still keep the tsx to help show that it's expected to actually be a TSX file (and can be if something like facebook/jscodeshift#332 is merged). Happy to remove this and keep it only class.input.js 😕

});

afterEach(() => {
jest.resetModules();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary to clear the jest.mock above, otherwise the following typescript block was ran with the flow parser because of the above mock. Jest Docs.

Thought about doing a beforeAll/afterAll since all the flow tests need the same mock, or promoting this afterEach to the top-level describe. Thought this was cleaner to run before/after every test and keep it specific to where it's needed but happy to change.

@skovy
Copy link
Contributor Author

skovy commented Jul 27, 2019

@threepointone RE: #228 (comment)

Here's an attempt at adding test cases for TypeScript. It works but it's not elegant. Not sure if it's better to have no tests or these hacky tests (relying on Object.assign and file extensions that end with .js). Let me know what you think! 👍

@threepointone
Copy link
Contributor

Looks good to me! Thank you for doing this, appreciate the weekend effort.

@threepointone threepointone merged commit 28b3da0 into reactjs:master Jul 27, 2019
@skovy skovy deleted the skovy/add-rename-lifecycle-typescript-test branch July 27, 2019 19:22
@skovy
Copy link
Contributor Author

skovy commented Jul 27, 2019

@threepointone definitely, thanks for the reviews! If any new transforms get added in future React releases feel free to tag me and I can test on a larger TypeScript codebase and add tests if needed 👍 For the most part the ASTs seem to be pretty similar but there was that one difference with the class methods. (haven't tried any of the other transforms so those might have different issues)

@FDiskas
Copy link

FDiskas commented Oct 1, 2019

npx react-codemod rename-unsafe-lifecycles --parser tsx

does not works in my case. All *.tsx files was skiped and componentWillReceiveProps was not renamed to UNSAFE_componentWillReceiveProps

For me looks like tests does nothing

@threepointone
Copy link
Contributor

@FDiskas please file a fresh issue with a reproducing example (like a git repo).

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.

3 participants