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

configure prettier & eslint #57

Merged
merged 1 commit into from
Mar 8, 2022
Merged

Conversation

polaroidkidd
Copy link
Contributor

I've added configs for prettier & eslint with a ts integration, along with husky and lint-staged.

In addition to extending 'eslint:recommended, react/recommendedand @typescript-eslint/recommended I've included most rules available in the eslint-typescript as warnings. I'm fairly sure we don't need all of them. My thought is that we can disable them by process of elimination or set ones we find important to error level.

I've also added devDependencies for eslint-filtered-fix and eslint-nibbl which allows you to fix eslint rules by rule, instead of fixing everything fixable at once.

I haven't run prettier over the src directory in this MR since I don't want the entire git history being overwritten due to formatting changes. I think there's a way to format it which lets us keep the line history somehow.

@derneuere
Copy link
Member

These seems to be an error when installing the dependencies.

librephotos-docker-frontend-1  | There might be a problem with the project dependency tree.
librephotos-docker-frontend-1  | It is likely not a bug in Create React App, but something you need to fix locally.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | The react-scripts package provided by Create React App requires a dependency:
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  |   "babel-eslint": "9.0.0"
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | Don't try to install it manually: your package manager does it automatically.
librephotos-docker-frontend-1  | However, a different version of babel-eslint was detected higher up in the tree:
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  |   /usr/src/app/node_modules/babel-eslint (version: 10.1.0) 
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | Manually installing incompatible versions is known to cause hard-to-debug issues.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | If you would prefer to ignore this check, add SKIP_PREFLIGHT_CHECK=true to an .env file in your project.
librephotos-docker-frontend-1  | That will permanently disable this message but you might encounter other issues.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | To fix the dependency tree, try following the steps below in the exact order:
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  |   1. Delete package-lock.json (not package.json!) and/or yarn.lock in your project folder.
librephotos-docker-frontend-1  |   2. Delete node_modules in your project folder.
librephotos-docker-frontend-1  |   3. Remove "babel-eslint" from dependencies and/or devDependencies in the package.json file in your project folder.
librephotos-docker-frontend-1  |   4. Run npm install or yarn, depending on the package manager you use.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | In most cases, this should be enough to fix the problem.
librephotos-docker-frontend-1  | If this has not helped, there are a few other things you can try:
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  |   5. If you used npm, install yarn (http://yarnpkg.com/) and repeat the above steps with it instead.
librephotos-docker-frontend-1  |      This may help because npm has known issues with package hoisting which may get resolved in future versions.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  |   6. Check if /usr/src/app/node_modules/babel-eslint is outside your project directory.
librephotos-docker-frontend-1  |      For example, you might have accidentally installed something in your home folder.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  |   7. Try running npm ls babel-eslint in your project folder.
librephotos-docker-frontend-1  |      This will tell you which other package (apart from the expected react-scripts) installed babel-eslint.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | If nothing else helps, add SKIP_PREFLIGHT_CHECK=true to an .env file in your project.
librephotos-docker-frontend-1  | That would permanently disable this preflight check in case you want to proceed anyway.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | P.S. We know this message is long but please read the steps above :-) We hope you find them helpful!
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | npm ERR! code ELIFECYCLE
librephotos-docker-frontend-1  | npm ERR! errno 1
librephotos-docker-frontend-1  | npm ERR! librephotos-frontend@0.1.0 start: `react-scripts start`
librephotos-docker-frontend-1  | npm ERR! Exit status 1
librephotos-docker-frontend-1  | npm ERR! 
librephotos-docker-frontend-1  | npm ERR! Failed at the librephotos-frontend@0.1.0 start script.
librephotos-docker-frontend-1  | npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
librephotos-docker-frontend-1  | 
librephotos-docker-frontend-1  | npm ERR! A complete log of this run can be found in:
librephotos-docker-frontend-1  | npm ERR!     /root/.npm/_logs/2022-03-05T20_50_01_590Z-debug.log
librephotos-docker-frontend-1 exited with code 1 

Deleting the node_modules folder and the package-lock.json did not work. I used the containerized dev environment, where "npm install --legacy-peer-deps" is called to install the dependencies. Could that be the reason or did I miss a step?

@polaroidkidd polaroidkidd force-pushed the dev branch 2 times, most recently from 6623a18 to 3984a80 Compare March 5, 2022 20:08
@polaroidkidd
Copy link
Contributor Author

i forgot to remove babel-eslint since it's not used in the final .eslintrc.js config. I removed it from the dependencies. I also added a .nvmrc file to freeze the node version (v12.22.5 is my current default and that seems to work)

@polaroidkidd polaroidkidd force-pushed the dev branch 2 times, most recently from c3dda84 to 538f78c Compare March 5, 2022 20:22
@polaroidkidd
Copy link
Contributor Author

just updated the dangling comma rule since it was complaining about them in imports as well.

@derneuere
Copy link
Member

Got a similar issue for eslint.

Got over it by bumping the react-script to 4.0.3 and downgrading eslint to 7.11.0. Now it's complaining about the config. I would guess that I have to bump react-script to version 5.0.0 in order for it to work with eslint 8.10.0. Will try that tomorrow :)

@polaroidkidd
Copy link
Contributor Author

Hmm, I'm going to test the stuff in the container and get back to you. Doesn't seem fair that you have to debug my PR. Should I close this PR and open a new one when it all works properly?

@derneuere
Copy link
Member

It's fine :D The problem is mostly caused by old dependencies. You can just continue to work in this pr :) Upgrading the version to react-scripts 5.0.0 worked. But now we have this issue in fomantic-ui: fomantic/Fomantic-UI#2027

@derneuere
Copy link
Member

Somebody opened up a pr, but it never got merged. But we could link to the pr git repo instead of the npm package.

@polaroidkidd
Copy link
Contributor Author

I'm fiddling about with the dependencies, splitting deps from devDeps and getting the lint and builds to work properly. Once I've got that up and running I'll have to test if everything still works as expected.

@polaroidkidd polaroidkidd force-pushed the dev branch 2 times, most recently from 8c5386d to 9a4a60d Compare March 6, 2022 15:53
@polaroidkidd
Copy link
Contributor Author

I managed to get it going by removing the "semantic-ui-css": "npm:fomantic-ui-css@^2.8.8", dependency and including the css via a CDN as described in their docs: https://react.semantic-ui.com/usage/#option-2-cdn-no-bundler

I wasn't sure that the users would appreciate a CDN dependency, but I noticed that there's also a unpkg dependency to leaflet.css.

I'm looking at patching it it via patch-package to remove the CDN dependency.

@polaroidkidd
Copy link
Contributor Author

I fixed the fomantic-ui-css issue using patch-package.

@polaroidkidd
Copy link
Contributor Author

I updated the prettier config to match the current code style as good as I could, but as far as I can tell the style isn't 100% in line everywhere. With the current configuration the new style will be applied whenever a file is changed with lint-staged. In my experience this isn't ideal. When someone submits a new PR, the changes will be mixed with those of the prettier config, making reviews difficult. I couldn't find a way to apply the new prettier config without loosing all git-blame information. One option would be to rebase this commit further into the history, but I'm not sure that that's a good idea either.

@derneuere
Copy link
Member

Installation of the dependencies and the changes that were needed in the dockerfiles works 👍
I have now a new issue: When I navigate to localhost:3000, I only get a blank page.

I had this issue here facebook/create-react-app#11779
Forwarding /ws in the proxy and upgrading the connection and setting "WDS_SOCKET_PORT=0" in the environment variables like here, made the error go away. but I still have only a blank page 😅 Does this only happen in the containerized environment?

I think formatting the codebase in one commit, would be a good enough solution. git blame does not work directly, but I still see in the history, who made the actual change.

@polaroidkidd
Copy link
Contributor Author

I only get a blank page.

I'm seeing it too now. There's some funky stuff going on with the routing. Once logged in it's directing from root to login and back again on every hard refresh. I'm going to continue to chip away at it until I can get the frontend/Dockerfile to build and run locally.

I think formatting the codebase in one commit, would be a good enough solution. git blame does not work directly, but I still see in the history, who made the actual change.

I guess that's an option, but the inline-git-blame will show the formatting commit by default. I'm not sure how important it is to keep this information at this point though. I'm with you on formatting it in one go though.

@polaroidkidd polaroidkidd force-pushed the dev branch 7 times, most recently from b0b152a to 8342c4f Compare March 8, 2022 07:53
@polaroidkidd polaroidkidd force-pushed the dev branch 4 times, most recently from 223ca74 to d8579bd Compare March 8, 2022 10:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derneuere
Copy link
Member

Looks good 👍

@derneuere derneuere merged commit 66abb16 into LibrePhotos:dev Mar 8, 2022
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