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

chore: add create-react-app test #22

Open
wants to merge 1 commit into
base: default
Choose a base branch
from
Open

Conversation

Gozala
Copy link
Owner

@Gozala Gozala commented Apr 20, 2021

Primary objective of this PR is:

  • Added a test case to ensure that users using create-react-app don't break as per fix: make esm mjs #21 (which fixed underlying issue).

Unfortunately this end up been huge time sink 😭. create-react-app, react-native etc... all bring various tools incompatible with each other or TS etc... All the other changes in this PR are duck tapes to work around those:

  • I had to move react-native test into test package otherwise it's deps broke create-react-app 🤷‍♂️
  • npm install from test packages caused node_module/@types/babel* stuff to appear that caused TS to fail. After hours of failed attempts to prevent those deps to appear and also failed attempts to get TS to ignore those I've gave up and added rm -rf ../../node_modules before test runs. Which seems to resolved the issues even if it's gross.

@somay I think this still works with react-native, but if you have time to take a look it would be greatly appreciated.

@somay
Copy link
Contributor

somay commented May 6, 2021

@Gozala Thank you for maintaining this helpful library with huge dedication!
I'll look and test it soon.

@somay
Copy link
Contributor

somay commented May 12, 2021

@Gozala I verified that it works on Android and iOS. No problem.

I also investigated dependency conflict btw react-native and create-react-app tests just for my curiosity.
It might be due to npm's semver resolution order (I didn't check documentations about the order, so it's my speculation).
yarn install just works, FYI.

The latest version of the react-native package has peerdep to react@17.0.1 (exact), and the latest of react itself is 17.0.2.
I'm speculating that npm firstly resolves semvers of dependencies without taking peerdeps into account (so react@^17.0.1 is resolved to 17.0.2) and then just checks if semvers of peerdeps ([email protected]) match to the resolved versions (so abort because they don't match).

Copy link
Contributor

@somay somay left a comment

Choose a reason for hiding this comment

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

I think that your decision to isolate react native test is actually good and feel sorry for not having done it on my own.
I think it's possible that packages used by react-native and create-react-app are much different and have some conflicts because React Native uses its own bundler (metro) instead of such as webpack.
I don't understand what happens when installing react-native and create-react-app at the same time.

On your second point, I'm currently failing to reproduce it and encountering no babel/TS issues even when I delete all rm -rf ../../node_modules.
In my environment npm install from the test packages installs packages to their own directories' node_modules/ and doesn't affect other test packages' behaviours.
So can you share the package-lock.json or other related information if you still can reproduce it?
I also want to suggest commiting package-lock.json to enhance test reproducibility if you don't have particular reasons against it.

The CI job failure on windows can be fixed by replacing npm's --prefix option with cd to test directory.
A bug of npm@6 causes it and it's reported here: npm/cli#3887
This bug causes npm to install some files to wrong locations and these files caused test failure.

And last, I'm very sorry for not helping you 7 month ago.
I'm in autism spectrum and often have difficulty understanding intentions of others, especially when communication is not so straight.
I'm actually willing to help you, so feel free to mention me at next time when you encounter react native issues.

@@ -20,14 +20,15 @@
"scripts": {
"test:node": "mocha test/test-*.spec.js",
"test:browser": "playwright-test test/test-*.js",
"test:react-native": "jest test/test-lib.react-native.spec.js",
"test:react-native": "npm test --prefix test/react-native",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can fix windows CI errors with this.

Suggested change
"test:react-native": "npm test --prefix test/react-native",
"test:react-native": "cd test/react-native && npm test",

@@ -20,14 +20,15 @@
"scripts": {
"test:node": "mocha test/test-*.spec.js",
"test:browser": "playwright-test test/test-*.js",
"test:react-native": "jest test/test-lib.react-native.spec.js",
"test:react-native": "npm test --prefix test/react-native",
"test:create-react-app": "npm test --prefix test/create-react-app",
Copy link
Contributor

Choose a reason for hiding this comment

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

same with the above suggestion.

Suggested change
"test:create-react-app": "npm test --prefix test/create-react-app",
"test:create-react-app": "cd test/create-react-app && npm test",

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.

2 participants