-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
First class support for CRA #102
Conversation
✅ Deploy Preview for jest-preview-library ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvh95 I just want to make sure the CLI still works even if the app is already ejected
'^(?!.*\\.(js|jsx|mjs|cjs|ts|tsx|css|json)$)': | ||
'jest-preview/transforms/fileCRA', | ||
}; | ||
jestConfig.transformIgnorePatterns = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do a check first to ensure transformIgnorePatterns
is an array and not nullish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need since we are targeting un-ejected CRA apps. See other comments for more details
cli/configCra.js
Outdated
(/** @type {string} */ pattern) => | ||
pattern !== '^.+\\.module\\.(css|sass|scss)$', | ||
); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO a check (if statement) is better than a try-catch block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. However, actually, Since I explained that jest-preview config-cra
target user with un-ejected app. So we don't even need to use try catch
for the simplicity of source code.
We can check more strictly when developing jest-preview config
null, | ||
true, | ||
); | ||
jestConfig.transform = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add these keys to the existing transform
instead of overwriting transform
like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This is intentional. We would like to remove '^.+\\.css$'
. Without removing this, it causes some issues that makes Sass not work in #101
@thanhsonng
The idea of The reason is after ejected, they can customize everything they want, we cannot provide a CLI that work with both un-ejected and ejected CRA. For example:
I think you want to support users who just ejected CRA without changing too much. But I don't think the common case, since after they ejected, they can do-whatever-they-want and we cannot anticipate that. But I agree with you about a universal CLI, which tries to do its best to support users setup Jest Preview easier. it can be jest-preview config instead of For now |
@thanhsonng I'm gonna merge this PR. Please let me know if I need to improve anything related to CRA support in the next PRs. Thanks |
Features
node scripts/test.js
instead ofreact-scripts test
(example)require('react-scripts/config/env');
instead ofrequire('../config/env');
jest.config.js
instead ofjest
object inpackage.json
(example)jest.config.js
(jest-preview config-cra
)scripts/test.js
(jest-preview config-cra
)jest-preview
receives argument (usecommander
)Chores