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

chore(styleguide): use 'tsup' instead of 'tsc' to compile styleguide #840

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

trm217
Copy link
Contributor

@trm217 trm217 commented May 17, 2024

This PR bundles the styleguide with tsup rather than tsc.
We bundle the code both as CJS and ESM to allow using it in our frontends, as well as the API.
Since the exports in key in the styleguides package.json now points to the bundled ESM rather than the ts files within the src directly (which was wrong to begin with 🙄), running styleguide + in parallel in dev-mode might feel less responsive (not that it was fast up until now 😛)

@trm217 trm217 temporarily deployed to www-republik-love May 17, 2024 13:46 Inactive
@trm217 trm217 had a problem deploying to republik-publikator-staging May 17, 2024 13:46 Failure
@trm217 trm217 temporarily deployed to api-republik-love May 17, 2024 13:47 Inactive
@trm217 trm217 temporarily deployed to republik-admin-staging May 17, 2024 13:47 Inactive
@trm217 trm217 temporarily deployed to republik-publikator-staging May 17, 2024 14:00 Inactive
@trm217 trm217 added app: admin Admin frontend server app: api GraphQL-API app: publikator Publikator frontend server package: styleguide Styleguide package and server labels May 17, 2024
@trm217 trm217 requested a review from jstcki May 17, 2024 14:08
@trm217 trm217 marked this pull request as ready for review May 17, 2024 14:11
Copy link
Contributor

@jstcki jstcki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, nice! Not sure why this would be less responsive than running tsc in dev mode, even if the import pointed to ts files.

The reason why it pointed to ts files was because we had the SG in transpilePackages. If we want to go through tsup anyway, then we should remove that as well.

packages/styleguide/tsup.config.ts Show resolved Hide resolved
packages/styleguide/tsup.config.ts Outdated Show resolved Hide resolved
@jstcki
Copy link
Contributor

jstcki commented May 22, 2024

Another thing I noticed: editor integration is broken now, Intellisense will point to the minified chunk file in dist/

Copy link

vercel bot commented May 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
admin-republik-ch ✅ Ready (Inspect) Visit Preview May 23, 2024 10:38am

@jstcki
Copy link
Contributor

jstcki commented May 29, 2024

Intellisense is working now but I get a new error during development, whenever I change something in styleguide:

image

This shows up during Fast Refresh but goes away after a page reload (also when Next.js reloads on its own)

@jstcki
Copy link
Contributor

jstcki commented May 29, 2024

Also this error now shows up in the console:

image

I think is because the .js loader should be different in dev than prod

@jstcki
Copy link
Contributor

jstcki commented May 29, 2024

For the minify option, this looks like a nicer approach than checking for NODE_ENV:

export default defineConfig((options) => {
  return {
    minify: !options.watch,
  }
})

@jstcki
Copy link
Contributor

jstcki commented May 29, 2024

Intellisense is working now but I get a new error during development, whenever I change something in styleguide:

image This shows up during Fast Refresh but goes away after a page reload (also when Next.js reloads on its own)

Seems to be better when clean is turned off (again, make this dependent on options.watch?)

@jstcki
Copy link
Contributor

jstcki commented May 29, 2024

Also, styleguide is still in next.config.js transpilePackages. I don't think that's necessary anymore.

@trm217
Copy link
Contributor Author

trm217 commented May 30, 2024

@jstcki seems to work fine with minifiy and clean being set to !options.watch.
Could you try again as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app: admin Admin frontend server app: api GraphQL-API app: publikator Publikator frontend server app: www Frontend server package: styleguide Styleguide package and server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants