Johnny88/fix/fixing tests #7653
Conversation
It seems like the linter is failing because of an error in the babel eslint package combined with dependencies being called in way that conflicts with yarn workspaces.
The yarn workspace command works locally but not in the script so I figured I would add lerna bootstrap
johnpangalos
requested review from
ianschmitz,
iansu,
mrmckeb and
petetnt
as code owners
Work so far
|
Adding lock file as it is the only thing that differs from my implementation and the one in the script
|
Okay so I can get eslint to run by running I have no idea why, but when I run just yarn things fail again... |
Seems like there is some issue with trying to run the script the way I would like, trying running eslint like the others that exist in the script.
This is a bit of a last resort, I want to see if anything else fails. I don't particularly like this solution
React scripts requires that babel eslint is 10.0.2
This was needed to fix for in false no unused var bug
So I was able to fix the tests:TL;DR: Not sure if it's better that the tests are fixed or that they are fixed in 100% right way. Gonna need some guidance from the contributors on that. Thanks in advance! Good News: Test pass and updated a dependency that was causing a false positive eslint error (for ins were causing false positive no-unused-vars) which will positively impact end users. Bad News: I really do not like how I solved the issue, seems like the only time babel eslint was installed properly (linking through workspaces) was when I installed it in the react-error-overlay repo itself. And when I would try and reinstall yarn from scratch to see if things were fixed, it would error babel eslint not found. I'm not sure if someone has more experience with yarn workspaces and package commands but any help in that area would be appriciated. |
Found a better solution by adding a few packages to eslint config react app, this should fix it so that it should always work in the tests. Using yarn workspaces or lerna in the scripts should probably be an improvement in another PR.
|
So I am pretty sure I found a better solution which only involves change some dependencies. Hopefully it passes CI/CD. |
|
So I like this solution better but I am still not 100% happy with it, adding duplicate dependencies between dev and peer seems like the wrong thing to do. I noticed that there may be some problems with yarn workspaces and peer dependencies going to look into it a bit more. |
UpdateThe current script doesn't seem to actually be running any of the eslint checking... I am not sure ./node_modules/.bin/eslint is working. I ran: and got 85 errors, which should be the equivalent of which gives out no logs leading me to believe that the latter is not actually working... I am starting to believe that the lint checking on PR has not been working for a while as the linter also did not pass on react-error-overlay (due to an error in babel-eslint but still.) |
|
Thanks a ton for looking into this @Johnny88. I've fixed this in #7662.
Indeed. I was able to get around it by bumping a few dependencies. There seems to be some strange yarn workspace module hoisting issues going on. Not quite clear on that, but it's resolved now.
the |
|
Ahhh that would make sense, no problem for looking into it. Thanks for fixing it! |
So the problem I've been getting when running the tests is that babel eslint is not "installed" in react error overlay when eslint is called in that repo.