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(Avatar): add fallback workaround to graphemer import class #1745

Closed
wants to merge 1 commit into from

Conversation

booc0mtaco
Copy link
Contributor

Summary:

Related to flmnt/graphemer#11, we have an issue in some consumers where the commonJS module is imported in such a way that we get the wrong object format. (instead of [Graphemer class] we get {default: [Graphemer class]}, which causes the code to fail).

Check for double-default wrapping as a workaround until graphemer is fixed. This will also unblock work on the theming tooling

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:
    • ensure tests pass locally
    • test with project that has the import problem, to ensure tests / storybook work properly

@booc0mtaco booc0mtaco requested a review from a team September 7, 2023 15:46
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

size-limit report 📦

Path Size
components 95.41 KB (+0.13% 🔺)
styles 32.77 KB (0%)

const splitter = new Graphemer();
// @see https://github.com/flmnt/graphemer/issues/11
// @ts-expect-error handles case where this library adds .default to the export inappropriately in CJS context
const G = Graphemer['default'] ? Graphemer.default : Graphemer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: might be able to use in and avoid the TS error

Suggested change
const G = Graphemer['default'] ? Graphemer.default : Graphemer;
const G = 'default' in Graphemer ? Graphemer.default : Graphemer;

Although I can't imagine any of this makes TS happy 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript HATES this one simple trick

😅

@ahuth ahuth mentioned this pull request Sep 7, 2023
4 tasks
@booc0mtaco booc0mtaco added the do not merge PRs that should not be merged (even if the build is green or there are approvals) label Sep 7, 2023
@booc0mtaco
Copy link
Contributor Author

We don't need this workaround anymore, as setting interop: auto in rollup did the trick 👍

cc @ahuth

@booc0mtaco booc0mtaco closed this Sep 8, 2023
@booc0mtaco booc0mtaco deleted the aholloway/x-fix-graphemer-cjs-import branch January 11, 2024 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs that should not be merged (even if the build is green or there are approvals)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants