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(config): break ui.config.ts into separate files #930

Merged
merged 5 commits into from
Nov 22, 2023
Merged

refactor(config): break ui.config.ts into separate files #930

merged 5 commits into from
Nov 22, 2023

Conversation

ineshbose
Copy link
Member

@ineshbose ineshbose commented Nov 8, 2023

πŸ”— Linked issue

makes progress on #877

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

While work on reducing bundle size automatically will continue, this is part of the foundation for it.

This will also provide a workaround for devs to exclude the files for components not being used as Tailwind's content configuration reads entire files.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

vercel bot commented Nov 8, 2023

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

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Nov 22, 2023 9:03am

@benjamincanac
Copy link
Member

benjamincanac commented Nov 21, 2023

@ineshbose I haven't forgotten this but it's quite tricky to merge this as it will break all existing PRs. Also, is this really the solution? There should be a better way to exclude unused components from the tailwind config automatically.

@ineshbose
Copy link
Member Author

@ineshbose I haven't forgotten this but it's quite tricky to merge this as it will break all existing PRs. Also, is this really the solution? There should be a better way to exclude unused components from the tailwind config automatically.

This isn't going to be the proper solution, but it is a step towards reaching it. Content detection in Tailwind is not automated right now, so just like nuxt/ui adds in the files:

ui/src/module.ts

Lines 158 to 162 in ceb2ed3

content: {
files: [
resolve(runtimeDir, 'components/**/*.{vue,mjs,ts}'),
resolve(runtimeDir, '*.{mjs,js,ts}')
],

we're going to have to consume them all. However, we most certainly do need to break the files down on a component-level if we are to make progress on this. When this is merged, users will at least have the option to specify the component files they don't use like so:

import { resolve } from 'pathe'

export default {
  content: [
    '!' + resolve('node_modules/@nuxt/ui/dist/runtime/ui.config/data/table.mjs'),
    '!' + resolve('node_modules/@nuxt/ui/dist/runtime/ui.config/forms/radioGroup.mjs'),
  ]
}

without this, all styles are in a singular file, so we either include it, or exclude it or do this similar approach above but in a complex way by creating an extractor function (not ideal).

All said, what would be really great for the next step of this PR, would be using unimport context during build to analyse imports and then do content configuration. Not to go out of depth and over-promise here, but this can require intensive time and research, so an automated solution will need to wait while the solution (imo) depends on this PR.

(Happy to continue discussion over Discord btw πŸ™‚)

@benjamincanac
Copy link
Member

I'm gonna make a release in the coming days, do you think it would be best to count this in?

@ineshbose
Copy link
Member Author

I'm gonna make a release in the coming days, do you think it would be best to count this in?

@benjamincanac I understand your point about breaking existing PRs, though we can also argue that implementing this change sooner for contributors to build upon would be better. I see 11 open feat PRs (some are features to existing components, so not touching ui.config, but we should also split the file to avoid potential future merge conflicts too), and fixing the conflict shouldn't be too difficult (just moving to a new file). I'll have to leave it for you to decide!

Copy link
Member

You make a good point, let's merge this then! Would you mind fixing the conflicts? If you have any doubt on what changed I can take care of it no worries 😊

@benjamincanac benjamincanac changed the title refactor(ui.config): break config into separate files refactor(config): break ui.config.ts into separate files Nov 21, 2023
@ineshbose
Copy link
Member Author

OK that's done. Ready to merge. @benjamincanac πŸ˜„

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.

None yet

2 participants