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

Linter should enforce style in w3console #510

Open
travis opened this issue Apr 26, 2023 · 3 comments
Open

Linter should enforce style in w3console #510

travis opened this issue Apr 26, 2023 · 3 comments
Assignees

Comments

@travis
Copy link
Member

travis commented Apr 26, 2023

It appears to be ignoring w3console at the moment, which means the code there is slowly drifting toward ugly - the mix of semicolon-terminated lines and non-semicolon-terminated lines is the most obvious smell but there are other issues as well.

As reported in #507 (comment)

@travis
Copy link
Member Author

travis commented Apr 28, 2023

I dug into this a bit - it turns out that turning on our standard linter (eg, tsc --build && eslint '**/*.{js,jsx,ts,tsx}') turns up a LOT of issues, especially the TypeScript compilation phase.

Many of the issues are easy to fix automatically, but many of them look like they are fairly tricky Preact-related typing issues. While we could invest time into fixing this, it doesn't seem especially "center of plate" for our team right now, especially since I'm already interested in moving us over to a more full-featured framework (see #419).

Given this, I'd like to propose we solve this one of two ways:

  1. Ditch preact and "rebase" w3console on top of the Vite react-ts starter template which has a linter setup by default that works well with Vite (the preact template, by contrast, has no linter configured): https://stackblitz.com/edit/vitejs-vite-hex6t4?file=index.html&terminal=dev

  2. Ditch Vite entirely and move to Next.js - I think this is the path of least resistance right now and wouldn't be too much of a lift - we'd be able to delete the routing logic that replicates default Next.js routing, so it should make things simpler.

@alanshaw @olizilla @Gozala thoughts?

@Gozala
Copy link
Contributor

Gozala commented May 2, 2023

I have no strong feeling on any of this. I would however propose another (perhaps) controversial solution to the problem. What if we just use prettier and and call it a day, it will address most issues and provide consistency without been nit picky on things that I personally hardly ever find useful.

We could even setup https://github.com/marketplace/actions/prettier-action to just automate reformatting at PR level

@travis
Copy link
Member Author

travis commented May 3, 2023

Ah so we actually were using prettier, but it didn't play nicely with eslint so I removed it:

cafefb0

The other issue is that one of the main problems here is that tsc --build is the part that's failing in w3console and using prettier wouldn't do anything to fix that - we need to either debug and fix (what appear to be) some fairly thorny type incompatibilities between React and Preact or just move w3console to React. Given that it seems like w3console is trending toward being the main UI we'll deploy on top of w3up, I'm more inclined to just move it to its own project and use Next.js rather than Vite - should mostly be a matter of copying src files into a new project because I've tried to build it in line with Next.js's major architectural decisions. Next.js has a more coherent/well supported type checking and linting story so this problem would mostly disappear with that move.

All that being said I think this is probably a decision we should make after we have a larger conversation about how we want to roll w3up out to web3.storage and nft.storage users because the "right" move is going to be heavily dependent on our plans for that.

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

No branches or pull requests

2 participants