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

fix(preservemodules): temporarily remove preserveModules #601

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

maiieul
Copy link
Contributor

@maiieul maiieul commented Jan 13, 2024

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests
  • Other

Why is it needed?

For two reasons:

Until QwikDev/qwik#5473 gets solved in Qwik, I say we temporarily disable preserveModules. We will have less performance (it should be about 11kb more instead of 40 now that the repo is cleaned up) in favor of more stability.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have ran pnpm changeset and documented my changes
  • I have add necessary docs (if needed)
  • Added new tests to cover the fix / functionality

Copy link

changeset-bot bot commented Jan 13, 2024

⚠️ No Changeset found

Latest commit: e138c29

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@shairez
Copy link
Contributor

shairez commented Jan 13, 2024

Thanks @maiieul
I personally prefer to fix the issue in Qwik first

My question now is - is this critical for the release of headless and fluffy, or is it just an optimization that could be done later?

@maiieul
Copy link
Contributor Author

maiieul commented Jan 13, 2024

I think it's a bit critical yes. Fluffy should provide the Accordion components named as "Accordion", and right now we can't. Also if we want to appear as a serious alternative to other frameworks even for UI, we need to have a docs site that feels more performant (which is not the case right now because of the CLS issues and the big bundle with shikiji. Besides, the namespace issue will be problematic for any other component where we'd like to reuse the same names for the styled components as the headless components.

For me it's having "preserveModules" that is an optimization that can be added later. It's also very easy to add back. For me the problem is that we don't know when this will be fixed in Qwik. Could be in 3 months but maybe 6 or even a year from now...

@maiieul
Copy link
Contributor Author

maiieul commented Jan 13, 2024

P.S. I think we need to prioritize our DX (fast development & no bugs) before slightly better perf for the end user for now.

Also I believe that the fact that we can't really use preserveModules with the current conditions should be pressing Misko or Manu to prioritize this before other issues. Helping library authors is vital to Qwik's ecosystem and I'm sure they understand that.

@PatrickJS
Copy link
Member

LGTM

@maiieul
Copy link
Contributor Author

maiieul commented Jan 14, 2024

If we are not merging this, then it also means we'll have to ask all our users at some point to change the name of their components - unless we can find a name that can stay, but I will always prefer "Accordion" to anything else.

So if we don't merge this, what prefix/suffix should we use?
We cannot use "Accordion"

So a few ideas:

Accordion$
QUIAccordion
$Accordion
AccordionQ

In my opinion Accordion$ or $Accordion look the cleanest. Maybe $Accordion would be less confusing in the sense that Accordion$ could let people think that it's useful for the optimizer? But also Accordion$ feels more natural. All in all I prefer "Accordion" ofc.

Usually things like "_Accordion" are for internal use, no? It feels awkward to me. $ is more in the spirit of Qwik if we decide to go down that path 😉

@shairez shairez merged commit 7127ab7 into qwikifiers:main Jan 16, 2024
5 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2024
@shairez
Copy link
Contributor

shairez commented Jan 16, 2024

merged
I've commented the lines instead of deleting them, and added #606 as reminder

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants