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

Refactor for Modernize 2024 #1249

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Refactor for Modernize 2024 #1249

wants to merge 9 commits into from

Conversation

varna
Copy link
Contributor

@varna varna commented Jul 3, 2024

Hello,

Tank you for Modernize 2024. I'm really glad that you adopted Vite. I think this was a great choice that could simplify the pipeline and increase community engagement.

I wanted to add few additional improvements:

Add .env example file and remove no longer needed code for access_token and simplify package.json scripts.

eslint

My VScode didn't really work out of the box after cloning repo. So I tried updating editor settings, recommendations and eslint to fix that issue. Updated mourner package and eslint to flat config. Some rules became redundant and some got renamed. There are some new default rules from eslint recommended rules that mourner uses, so I had to add some rules to avoid linting error and changing thousands of lines in project. I extremely recommend to remove these a bit later:

      'strict': 0,
      'no-else-return': 0,
      'no-promise-executor-return': 0,
      'no-await-in-loop': 0,

vite

The thing is that Vite uses esbuild for dev mode and rollup for build. So using either of those on their own is kind of redundant. That's why I removed rollup. And I also simplified package scripts.

I setup Vite to work in a library mode, so you don't need to build the lib to run a html file like debug or bench. Currently both start with npm run dev. build creates umd and esm packages, but I swapped their naming a bit to match current behaviour of Draw. I recommend removing this custom naming in favour of using ESM as default and renaming UMD to .umd.cjs (because UMD is only needed for CDN). And releasing this as v2 after beta testing.

testing and bench

You got a lot of tests 🤣

I recommend using Vitest for testing. Everyone who uses it loves it and it has amazing VSCode integration. In theory it should be enough to replace all imports from node to vitest. But in practise (after trying on one file) only 80% of tests migrate that easily. I need to do more tweaking, like replacing sinon and removing AbortSignal.

Also, moving bench to vitest might be good idea (that's why I haven't finished refactoring it yet). But I haven't done benches before, might be a good practise for me. But idk if I have time for it because my boss already shouting at me for being a commie that wastes time on OS.

feedback

So, I wanted to get opinions and feedback on my ideas and proposed changes.

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.

1 participant