vercel / speed-insights

Vercel Speed Insights package
https://vercel.com/docs/speed-insights
Apache License 2.0
63 stars 9 forks source link

Fix astro package entrypoint types #55

Closed MoustaphaDev closed 9 months ago

MoustaphaDev commented 9 months ago

πŸ““ What's in there?

Fixes https://github.com/vercel/speed-insights/issues/54. The types field of the astro export didn't point to any existing file, so it couldn't be resolved. I'm not sure why the fix in https://github.com/vercel/speed-insights/pull/41 supposedly worked for some people, but this should hopefully fix it for everyone.

πŸ§ͺ How to test?

# build the package
pnpm -F @vercel/speed-insights build

# cd into the astro example app
cd apps/astro

# install typescript and @astrojs/check
pnpm i -D typescript @astrojs/check

# run `astro check` 
pnpm astro check

This should work now, but before it caused an error

Before ```sh > astro "check" 22:55:55 Types generated 473ms 22:55:55 [check] Getting diagnostics for Astro files in /home/coding/speed-insights/apps/astro... src/components/BaseHead.astro:5:27 - error ts(2307): Cannot find module '@vercel/speed-insights/astro' or its corresponding type declarations. 5 import SpeedInsights from '@vercel/speed-insights/astro'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/components/BaseHead.astro:49:12 - warning ts(6133): 'speedInsightsBeforeSend' is declared but its value is never read. 49 function speedInsightsBeforeSend(data){ ~~~~~~~~~~~~~~~~~~~~~~~ Result (14 files): - 1 error - 0 warnings - 1 hint ```
After ```sh > astro "check" 22:58:46 Types generated 425ms 22:58:46 [check] Getting diagnostics for Astro files in /home/coding/speed-insights/apps/astro... src/components/BaseHead.astro:49:12 - warning ts(6133): 'speedInsightsBeforeSend' is declared but its value is never read. 49 function speedInsightsBeforeSend(data){ ~~~~~~~~~~~~~~~~~~~~~~~ Result (14 files): - 0 errors - 0 warnings - 1 hint ```

The type error was also surfaced in the IDE, but it's resolved fine now

Before ![Screenshot from 2024-02-12 23-54-35](https://github.com/vercel/speed-insights/assets/81974850/2a39e13c-c6f0-47a5-b9e7-9b98299b9053)
After ![Screenshot from 2024-02-13 01-27-43-edit](https://github.com/vercel/speed-insights/assets/81974850/2460cb3e-3532-4914-b5fd-2b588b8d710b)
vercel[bot] commented 9 months ago

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
speed-insights-astro βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 14, 2024 4:30pm
speed-insights-nextjs βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 14, 2024 4:30pm
speed-insights-nuxt βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 14, 2024 4:30pm
speed-insights-remix βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 14, 2024 4:30pm
speed-insights-sveltekit βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 14, 2024 4:30pm
speed-insights-vue βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 14, 2024 4:30pm
MoustaphaDev commented 9 months ago

Would love if you can have a look at this @tobiaslins (or any other maintainer)!

rochajulian commented 9 months ago

Would love if you can have a look at this @tobiaslins (or any other maintainer)!

Second this

feugy commented 9 months ago

Hello @MoustaphaDev, and thanks for your contribution!

I'm annoyed, because I can't reproduce your error on the main branch.

I do see the warning, but not the error. ```shell speed-insights on ξ‚  main [$!?] via  v18.17.1 on ☁️ (us-east-1) ❯ rm -rf packages/web/dist speed-insights on ξ‚  main [$!?] via  v18.17.1 on ☁️ (us-east-1) ❯ pnpm i Scope: all 9 workspace projects Lockfile is up to date, resolution step is skipped Already up to date . prepare$ husky install β”‚ husky - Git hooks installed └─ Done in 180ms packages/web postinstall$ node scripts/postinstall.mjs └─ Done in 50ms apps/nuxt postinstall$ nuxt prepare β”‚ βœ” Types generated in .nuxt └─ Done in 1.1s Done in 2.2s speed-insights on ξ‚  main [$!?] via  v18.17.1 on ☁️ (us-east-1) took 2s ❯ pnpm -F @vercel/speed-insights build > @vercel/speed-insights@1.0.9 build /Users/damien/dev/speed-insights/packages/web > tsup && pnpm copy-astro CLI Building entry: {"index":"src/generic.ts"} CLI Using tsconfig: tsconfig.json CLI Building entry: {"index":"src/nextjs/index.tsx"} CLI Using tsconfig: tsconfig.json CLI Building entry: {"index":"src/nuxt/index.ts"} CLI Using tsconfig: tsconfig.json CLI Building entry: {"index":"src/remix/index.tsx"} CLI Using tsconfig: tsconfig.json CLI Building entry: {"index":"src/sveltekit/index.ts"} CLI Using tsconfig: tsconfig.json CLI Building entry: {"index":"src/vue/index.ts"} CLI Using tsconfig: tsconfig.json CLI Building entry: {"index":"src/react/index.tsx"} CLI Using tsconfig: tsconfig.json CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI tsup v7.2.0 CLI Using tsup config: /Users/damien/dev/speed-insights/packages/web/tsup.config.js CLI Target: node16 CLI Target: node16 CLI Target: node16 CLI Target: node16 CLI Target: node16 CLI Target: node16 CLI Target: node16 CLI Cleaning output folder ESM Build start CJS Build start CLI Cleaning output folder ESM Build start CJS Build start CLI Cleaning output folder ESM Build start CJS Build start CLI Cleaning output folder ESM Build start CJS Build start CLI Cleaning output folder ESM Build start CJS Build start CLI Cleaning output folder ESM Build start CJS Build start CLI Cleaning output folder ESM Build start CJS Build start ESM dist/index.mjs 3.12 KB ESM dist/index.mjs.map 10.65 KB ESM ⚑️ Build success in 76ms CJS dist/index.js 4.19 KB CJS dist/index.js.map 10.71 KB CJS ⚑️ Build success in 84ms CJS dist/react/index.js 4.61 KB CJS dist/react/index.js.map 11.81 KB CJS ⚑️ Build success in 84ms ESM dist/react/index.mjs 3.55 KB ESM dist/react/index.mjs.map 11.77 KB ESM ⚑️ Build success in 89ms CJS dist/sveltekit/index.js 4.15 KB CJS dist/sveltekit/index.js.map 11.63 KB CJS ⚑️ Build success in 96ms CJS dist/remix/index.js 5.69 KB CJS dist/remix/index.js.map 12.96 KB CJS ⚑️ Build success in 98ms CJS dist/vue/index.js 4.90 KB CJS dist/vue/index.js.map 12.61 KB CJS ⚑️ Build success in 100ms ESM dist/next/index.mjs 4.37 KB ESM dist/next/index.mjs.map 13.46 KB ESM ⚑️ Build success in 101ms ESM dist/vue/index.mjs 3.85 KB ESM dist/vue/index.mjs.map 12.58 KB ESM ⚑️ Build success in 100ms ESM dist/nuxt/index.mjs 3.85 KB ESM dist/nuxt/index.mjs.map 12.60 KB ESM ⚑️ Build success in 100ms ESM dist/sveltekit/index.mjs 3.06 KB ESM dist/sveltekit/index.mjs.map 11.56 KB ESM ⚑️ Build success in 102ms ESM dist/remix/index.mjs 4.04 KB ESM dist/remix/index.mjs.map 12.84 KB ESM ⚑️ Build success in 100ms CJS dist/nuxt/index.js 4.92 KB CJS dist/nuxt/index.js.map 12.63 KB CJS ⚑️ Build success in 101ms CJS dist/next/index.js 6.07 KB CJS dist/next/index.js.map 13.57 KB CJS ⚑️ Build success in 103ms DTS Build start DTS Build start DTS Build start DTS Build start DTS Build start DTS Build start DTS Build start DTS ⚑️ Build success in 1211ms DTS dist/index.d.mts 2.17 KB DTS dist/index.d.ts 2.17 KB DTS ⚑️ Build success in 1480ms DTS dist/next/index.d.mts 1.13 KB DTS dist/next/index.d.ts 1.13 KB DTS ⚑️ Build success in 1609ms DTS dist/react/index.d.mts 1.23 KB DTS dist/react/index.d.ts 1.23 KB DTS ⚑️ Build success in 1694ms DTS dist/remix/index.d.mts 1.07 KB DTS dist/remix/index.d.ts 1.07 KB DTS ⚑️ Build success in 1731ms DTS dist/sveltekit/index.d.mts 1.08 KB DTS dist/sveltekit/index.d.ts 1.08 KB DTS ⚑️ Build success in 1996ms DTS dist/nuxt/index.d.mts 61.00 B DTS dist/nuxt/index.d.ts 61.00 B DTS ⚑️ Build success in 2041ms DTS dist/vue/index.d.mts 61.00 B DTS dist/vue/index.d.ts 61.00 B > @vercel/speed-insights@1.0.9 copy-astro /Users/damien/dev/speed-insights/packages/web > cp -R src/astro dist/ speed-insights on ξ‚  main [$!?] via  v18.17.1 on ☁️ (us-east-1) took 3s ❯ cd apps/astro speed-insights/apps/astro on ξ‚  main [$!?] is πŸ“¦ v0.0.1 via  v18.17.1 on ☁️ (us-east-1) ❯ pnpm i -D typescript @astrojs/check ../../packages/web |  WARN  deprecated abab@2.0.6 ../../packages/web |  WARN  deprecated domexception@4.0.0 ../.. | Progress: resolved 1874, reused 1749, downloaded 0, added 0, done ../.. prepare$ husky install β”‚ husky - Git hooks installed └─ Done in 247ms ../../packages/web postinstall$ node scripts/postinstall.mjs └─ Done in 47ms ../nuxt postinstall$ nuxt prepare β”‚ βœ” Types generated in .nuxt └─ Done in 1.2s Done in 6.2s speed-insights/apps/astro on ξ‚  main [$!?] is πŸ“¦ v0.0.1 via  v18.17.1 on ☁️ (us-east-1) took 6s ❯ pnpm astro check > astro@0.0.1 astro /Users/damien/dev/speed-insights/apps/astro > astro "check" 12:21:54 Types generated 170ms 12:21:54 [check] Getting diagnostics for Astro files in /Users/damien/dev/speed-insights/apps/astro... src/components/BaseHead.astro:49:12 - warning ts(6133): 'speedInsightsBeforeSend' is declared but its value is never read. 49 function speedInsightsBeforeSend(data){ ~~~~~~~~~~~~~~~~~~~~~~~ Result (14 files): - 0 errors - 0 warnings - 1 hint ```

The hack implemented #41 comes from the Astro core team (see the PR desc for an explanation). Let's try your fix. I'll release it, and we'll revert in case we receive some bug reports from other Astro users.

feugy commented 9 months ago

@MoustaphaDev , @rochajulian, I've released 1.0.10. Please try it out and let us know.

MoustaphaDev commented 9 months ago

I'm so confused as well, now I can't even reproduce this error anymore with 1.0.9. The Astro language server and typescript plugin were updated yesterday, so I suspect that might have helped solve the problem. 1.0.10 is working as well. I guess reverting when we find any bug, as you said, would be best πŸ€·β€β™‚οΈ