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

Vite 5 and updated all packages with eslint flat config #274

Closed
wants to merge 19 commits into from

Conversation

TechAkayy
Copy link
Collaborator

@TechAkayy TechAkayy commented Jul 27, 2024

Description 📖

This pull request updates Iles & it's packages to Vite 5 along with all it's dependencies (had partial success, but with help we can fix it).

Commit-1 - Updated dependencies along with any API changes in their major versions (not many). Also, updated to eslint 9 with a flag config eslint.config.mjs. Installed @vue-macros/reactivity-transform to keep $-style APIs.

Commit-2 - Minor types references updated in two packages' package.json.

Commit-3/4 - Removed @vue-macros/reactivity-transform and updated to standard vue APIs. This can be ignored if you want to keep the $-style APIs.

There are still some minor hiccups for which I need help. Details with screenshots below.

Screenshots 📷

pnpm install installs without any peer dependency errors except for @mussi-eslint-config related. I guess this is your package @ElMassimo, and wondering what's the purpose of it? Is it still required in the new eslint 9 flat config way?

pnpm build:all builds fine.

pnpm dev throws some types warnings, looks harmless.
image

The reason I removed reactivity-transform macro is because there seems to be some issue with it. Notice the missing semicolon after .../BackTick.tsx'. It was turned into a blocker for me.
image

Key Issue:

Client Script Block seem to not work. When used,

<script client:load lang="ts">
console.log('Powered by îles 🏝', 'https://iles-docs.netlify.app')
</script>

I could trace it to an error logged by vue, something around missing end tag.
image

I have added three // @ts-ignore in https://github.com/TechAkayy/iles/blob/417c641586484ede92f1bf12579d517479294703/packages/iles/src/node/plugin/wrap.ts#L50 which is related to the possibility of template.ast to be undefined. Type wasn't reconciling as ast showed up as RootNode, while some functions that it was pass through was expecting ElementNode.

@TechAkayy TechAkayy mentioned this pull request Jul 27, 2024
@TechAkayy
Copy link
Collaborator Author

One important thing - because of the peer deps issue with @mussi/eslint-config, I couldn't lint properly.

@ElMassimo
Copy link
Owner

Hi Ahmed!

Appreciate you sharing this pull request.

Regarding the ESLint config, I'd be ok using something like @antfu/eslint-config and tweak a few rules which align more with standardjs.

For now feel free to ignore any lint issues, or disable that workflow.

I'll follow up on this when I have some time, thanks!

@TechAkayy
Copy link
Collaborator Author

TechAkayy commented Aug 1, 2024

Completed my PR for the update, here is a quick summary:

  • Configured @antfu/eslint-config (thanks @antfu) and removed @mussi/eslint-config. Main package.json has a minor addition to allow eslint9 until the time rest of the ecosystem catchup, see Anthony Fu's comment here. Added these rules to the flat config as these are generously exempted in the current codebase.
	'unused-imports/no-unused-vars': 'off',
	'style/max-statements-per-line': 'off',
	'no-console': 'off',
	'node/prefer-global/process': 'off',
	'antfu/no-import-dist': 'off',
  • Added lint:fix to all packages, and completed lint fixes for the whole codebase (in a seperate commit to track). For errors that required manual intervention, I have applied non-destructive eslint-disable-* for specific lines so that the code doesn't change in any way (unless types changed in major version of dependencies). We should ideally remove these going forward.
  • Updated pnpm to latest packageManager (thanks for the reminder @userquin). You might encounter this pnpm issue that might look like a vite type (PluginOption) issue.
  • Vite 5 breaking change - manifest files are now within a .vite subfolder within build.outDir.
  • Vite 5 - enabled cssCodeSplit in client bundles without which css wasn't getting extracted.
  • Vue 3.4 breaking change - template ast had a type change from ElementNode to RootNode (this spot) as part of performance enhancement - details here. This required wrap logic to be refactored (the three @tsignores in my original above post) as we use this internal api in packages/iles/src/node/plugin/wrap.ts
  • Vue 3.4 - Though not documented, custom blocks naming has to be in kebab-case, so changed scriptClient to script-client in Vite transform hook, without which vue core compiler throws an error (screenshot in my first post above). The validation probably was added later in the underlying compiler.
  • Both docs & blog repos dev & built nicely (windicss version)
  • Migrated the docs & blog to Unocss using Wind preset, dev & built nicely.

Only thing I couldn't solve is - in the blog, the one-piece image doesn't seem to be optimised, probably related to the underlying vite plugin, or in its usage (was there any breaking changes @ElMassimo?) - https://github.com/ElMassimo/vite-plugin-image-presets.

For the users updating iles, in their apps, with this update, they will have to:

  • remove version pinning of vue & vite in their package.json (if applicable). TODO: We might need to add vue & vite latest as peer deps as part of the update?
  • add type: module to their package.json and manage any uncompatible commonjs vite plugins if they show trouble, as vite 5 is esm-first
  • breaking: migrate off reactivity-transform ($computed -> computed with .value on refs, etc) or use the macro.

Might be worth spinning out a prerelease verison for our users to test after your review & if it works nicely at your end. I did test on a few samples at my end, worked nicely 😄 Thanks 👍

@TechAkayy
Copy link
Collaborator Author

TechAkayy commented Aug 2, 2024

With Vite 5 deprecating cjs node api, I have refactored tryInstallModule to tryImportOrInstallModule and use dynamic import() which works with both cjs & esm modules even when a package doesn't have main or exports defined in it's package.json. This takes care of packages like the svelte vite plugin that is esm only.

Also, antfu's install-pkg uses ezspawn which is not well maintained, and is currently broken with one of it's downstream package. So, I have taken inspiration from that and have used a childprocess exec to achieve the same in a simple way, we still use that package for detecting package manager. Auto install of iles modules works smooth with this, and I can see the installation succeeding, but testing on a local project (outside the playground folder) by pnpm linking the iles repo has been hard (probably a pnpm + monorepo challenge like in this issue).

@ElMassimo
Copy link
Owner

Hi @TechAkayy!

Would you enable me to push updates to this branch in your fork?

@TechAkayy
Copy link
Collaborator Author

Hi @TechAkayy!

Would you enable me to push updates to this branch in your fork?

Just invited you @ElMassimo. Thanks!

Good idea creating a fork without linting. Please let me know if you require anymore follow-ups from me. Cheers!

@ElMassimo ElMassimo changed the base branch from vite-5 to main November 10, 2024 02:16
@ElMassimo
Copy link
Owner

@TechAkayy Thanks again for diving into this!

I've completed the upgrades in #281, and the CI is green once again.

I've cherry-picked the last 3 commits here and moved them to:

so that we can define the API for enhanceIslands, which would be a nice addition to îles.

@ElMassimo ElMassimo closed this Nov 10, 2024
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