florian-lefebvre / astro-integration-kit

A package that contains utilities to help you build Astro integrations.
https://astro-integration-kit.netlify.app
MIT License
46 stars 8 forks source link

fix: update addVitePlugin plugin type to PluginOption #84

Closed ktym4a closed 3 months ago

ktym4a commented 3 months ago

Close #83

netlify[bot] commented 3 months ago

Deploy Preview for astro-integration-kit ready!

Name Link
Latest commit 9cd31c794b097af7c7715a473ba3697fcf571b9c
Latest deploy log https://app.netlify.com/sites/astro-integration-kit/deploys/65f98daa49cd420008eb1978
Deploy Preview https://deploy-preview-84--astro-integration-kit.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ktym4a commented 3 months ago

Can you use vite pwa in the playground to make sure it works?

I just check by seeing this.

VITE PLUGINS [
  'rollup-plugin-astro-tailwind-config-viewer',
  'vite-plugin-test-integration',
  'vite-plugin-test-integration',
  'vite-plugin-pwa',
  'vite-plugin-pwa:info',
  'vite-plugin-pwa:build',
  'vite-plugin-pwa:dev-sw',
  'vite-plugin-pwa:pwa-assets',
  'vite-plugin-test-integration-1',
  'vite-plugin-test-integration-2',
  'vite-plugin-test-react-plugin',
  'vite-plugin-test-react-plugin-1',
  'vite-plugin-test-preact-plugin',
  'vite-plugin-test-svelte-plugin',
  'vite-plugin-test-solid-plugin'
]

Is this enough?

ktym4a commented 3 months ago

Actually, there is still type error (but not like before).

Screenshot 2024-03-19 at 00 32 00

and If I run pnpm up -r --workspace, it's gone.

BryceRussell commented 3 months ago

This could be fixed in a separate PR but now that the type version/devDep mismatch for vite is fixed, this type could be replaced with Plugin<any> https://github.com/florian-lefebvre/astro-integration-kit/blob/main/package/src/plugins/has-vite-plugin.ts#L8-L11

ktym4a commented 3 months ago

I probably shouldn't comment here, but addIntegration also seems to have the type version/devDep mismatch.

Screenshot 2024-03-19 at 08 27 34
Adammatthiesen commented 3 months ago

I probably shouldn't comment here, but addIntegration also seems to have the type version/devDep mismatch.

Screenshot 2024-03-19 at 08 27 34

thats expected if you are not running the same version of astro as AIK has specified, it does still work though with all my integrations atleast... AIK has specified the hooks at astro v4.4.1 your logs are saying your integration is at 4.4.6, I actually have my integrations tagged at the same minimum under "peerDependencies" as AIK for this reason. example

ktym4a commented 3 months ago

Yes, but... All of this is within this monorepo now...

florian-lefebvre commented 3 months ago

I thought types would not break until you're using astro 4.5. Those types issues are truly a PITA! But we can track in a dedicated issue, feel free to create one! Can you just update the type here (https://github.com/florian-lefebvre/astro-integration-kit/blob/main/package/src/plugins/has-vite-plugin.ts#L8-L11) to PluginOption as well? Then I'll just for Luiz to answer on his comment above and we're good to go with this PR

ktym4a commented 3 months ago

Sorry, But I don't have a firm understanding of this, Should I change just here(https://github.com/florian-lefebvre/astro-integration-kit/blob/main/package/src/plugins/has-vite-plugin.ts#L8-L11)? or do I change all of AstroConfig["vite"]["plugins"]?

BryceRussell commented 3 months ago

@ktym4a You can replace the lines 8-11 with Plugin<any> so that it becomes Set<Plugin<any>>, the other type AstroConfig["vite"]["plugins"] can stay

ktym4a commented 3 months ago

I changed it as @BryceRussell said. But, sorry I do not have a clear understanding of this, so please let me know if I am wrong in any way.

BryceRussell commented 3 months ago

Looks good to me! Thanks! 🚀