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

Error thrown while generating error message for missing asset-type #450

Closed
adierkens opened this issue Jul 26, 2024 · 8 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@adierkens
Copy link
Member

Debugging an issue with assets missing from the registry, and the fuse search is obfuscating the real error message
CleanShot 2024-07-25 at 20 47 17

CleanShot 2024-07-25 at 20 47 23

@adierkens adierkens added the bug Something isn't working label Jul 26, 2024
@KetanReddy
Copy link
Member

Huh, that's strange. I'm surprised a test didn't catch that.

@KetanReddy
Copy link
Member

I was also surprised that TypeScript didn't flag that as an issue, but it looks like we don't have the check for that turned on. So we should probably enable noUncheckedIndexedAccess

@adierkens
Copy link
Member Author

I was able to determine the root-cause of the original error -- but my guess is that nothing in the registry came up as a similar matching name and it returned undef

I also noticed our bundle size grow ~15kb with the introduction of fuse.js:

https://bundlephobia.com/package/@player-ui/react@0.8.0-next.2 https://bundlephobia.com/package/@player-ui/react@0.8.0-next.4
CleanShot 2024-07-25 at 21 43 18 CleanShot 2024-07-25 at 21 43 45

I would try to find something smaller to replace it with, put it behind a DEBUG flag, or rip it out entirely

@KetanReddy
Copy link
Member

Maybe fuzzysort? 5kb isn't bad if we do want to support those kind of suggestions but since we don't really have dev/prod bullds it would have to always get included. https://github.com/farzher/fuzzysort

@adierkens
Copy link
Member Author

adierkens commented Jul 26, 2024

We likely don't need a full search library, something that computes the levenshtein distance would be enough to make a suggestion

https://www.npmjs.com/package/leven
https://www.npmjs.com/package/@laboralphy/did-you-mean

@cehan-Chloe
Copy link
Collaborator

cehan-Chloe commented Jul 26, 2024

@adierkens thanks for capturing this issue!
Questions:

  1. Could you share the asset info when you debugging the asset missing in registry? I'm surprised that typescript check and test cases didn't capture this issue. Will add new test cases for it.
  2. How we check the bundle size before merge pr? https://www.npmjs.com/package/fuse.js?activeTab=readme i was checking the fuse.js but it only has unpacked size

Action item:

  1. Replace fuse.js to avoid the bundle size increase
  2. Add noUncheckedIndexedAccess
  3. Add test cases for this scenario
    I was able to determine the root-cause of the original error -- but my guess is that nothing in the registry came up as a similar matching name and it returned undef

@cehan-Chloe
Copy link
Collaborator

cehan-Chloe commented Jul 26, 2024

ok i'm able to reproduce with the test case below and can confirm it's because no similar type/key name in registry fuse search will return empty array... https://www.fusejs.io/demo.html also tested in their demo page if i use any char like "8" which doesn't exist in any keys.

  const assetDef = {
    asset: {
      id: "bar-id",
      type: "bar",
    },
  } as unknown as AssetType;

  const registry: AssetRegistryType = new Registry([
    [{ type: "foo", key: "foo-key" }, () => <div>foo</div>],
    [{ type: "a", key: "test" }, () => <div>bar</div>],
  ]);

@adierkens
Copy link
Member Author

@cehan-Chloe You can use bundlephobia to check the canary version of the Player packages on your PR (similar to what was done in #432)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants