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

🩹 (radix-ui) NICE-97 reduced Anchor 🔗 [b] #2398

Merged
merged 4 commits into from
Mar 27, 2024
Merged

🩹 (radix-ui) NICE-97 reduced Anchor 🔗 [b] #2398

merged 4 commits into from
Mar 27, 2024

Conversation

JeromeFitz
Copy link
Owner

@JeromeFitz JeromeFitz commented Mar 27, 2024

Move AnchorUnstyled to be Anchor.


No idea why this does not happen on localized build but does on hosting.

Current

import { Flex } from '@radix-ui/themes'
  • PRO: More legible; Less up-keep on dist changes
  • CON: !tree-shaking

Preferred

import { Flex } from '@radix-ui/themes/dist/esm/components/flex.js'
  • PRO: Tree-shaking (49 KiB savings in unused JavaScript)
  • CON: Have to call every component like this to take advantage

If we call this directly (./dist/esm/components/flex.js) there is an intermittent error triggered via routing.

This looks to be due to flex.props and the parseJustifyValue being created after it is called.

  • Which I do not htink should be a problem, but perhaps with the way this is minified / prepared for production does matter for the order of the operations

This PR is more information gathering to be able to explain this issue and see if there is a fix,
or instead shelve and point to this as a discussion point for a later date.


Update: I actually think this is a routing issue, not a radix-ui/themes one. It just looks like it because it is triggering 404. Not sure. If anything a new problem has come up 😆

If we call this directly there is an intermittent
 issue with `flex.props` which looks due to
 `parseJustifyValue` being called before it is created

Though that ... really should not be a problem. But
 believe when this gets minified it can (or has).

This is more information gathering.
@JeromeFitz JeromeFitz added the 🛑️ Do Not Merge Do not merge until further notice label Mar 27, 2024
@JeromeFitz JeromeFitz changed the title 🩹 (radix-ui) NICE-97 Flex (parseJustifyValue) test [b] 🩹 (radix-ui) NICE-97 direct Flex; reduced Anchor [b] Mar 27, 2024
What the heck cannot replicate locally
But can replicate on Vercel
@JeromeFitz JeromeFitz changed the title 🩹 (radix-ui) NICE-97 direct Flex; reduced Anchor [b] 🩹 (radix-ui) NICE-97 reduced Anchor 🔗 [b] Mar 27, 2024
@BotJerome
Copy link
Collaborator

This pull request has been deployed to Vercel.

Latest commit: c661289
✅ Preview: https://jeromefitzgerald-w5jysyr8m-jerome5.vercel.app
🔍 Inspect: https://vercel.com/jerome/jeromefitzgerald/8rrY217ukztaM7JJ5vxAQjdxcW7n

View Workflow Logs

@JeromeFitz JeromeFitz added 🥳️ LGTM Automerge: Let's Get This Merged and removed 🛑️ Do Not Merge Do not merge until further notice labels Mar 27, 2024
@kodiakhq kodiakhq bot merged commit 1692720 into main Mar 27, 2024
3 checks passed
@kodiakhq kodiakhq bot deleted the NICE-97 branch March 27, 2024 17:40
JeromeFitz added a commit that referenced this pull request Mar 27, 2024
Move `AnchorUnstyled` _to be_ `Anchor`.

----

No idea why this does not happen on localized build but does on hosting.
----

Current

```tsx
import { Flex } from '@radix-ui/themes'
```

- PRO: More legible; Less up-keep on `dist` changes
- CON: !tree-shaking

Preferred

```tsx
import { Flex } from '@radix-ui/themes/dist/esm/components/flex.js'
```
- PRO: Tree-shaking (`49 KiB` savings in unused JavaScript)
- CON: Have to call every component like this to take advantage

---

If we call this directly (`./dist/esm/components/flex.js`) there is an intermittent error triggered via routing.

This looks to be due to `flex.props` and the `parseJustifyValue` being created _after_ it is called.
- Which I do not htink _should_ be a problem, but perhaps with the way this is minified / prepared for production does matter for the order of the operations

This PR is more information gathering to be able to explain this issue and see if there is a fix,
 or instead shelve and point to this as a discussion point for a later date.

 ----

 **Update:** I actually think this is a routing issue, not a `radix-ui/themes` one. It just looks like it because it is triggering `404`. Not sure. If anything a new problem has come up 😆
♻️ (react) unnecessary calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🥳️ LGTM Automerge: Let's Get This Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants