Skip to content

Conversation

@jake-slashid
Copy link
Contributor

@jake-slashid jake-slashid commented Jun 20, 2024

Description

YouTrack issue

Issue:
If anonymous users is turned on but storage is left default, it will be memory which means the anonymous users is lost each time the page refreshes.

Fix:

  • Set storage to localStorage by default if anonymous users enabled
  • When new default is invoked, emit warning to set storage explicitly
  • Warn if storage is explicitly set to memory while anonymous users enabled

@github-actions
Copy link
Contributor

Bundle size comparison

Name +/- Base Current +/- gzip Base gzip Current gzip
main.js +0.11% 367.24 kB 367.63 kB +0.17% 96.83 kB 97.00 kB
style.css = 37.52 kB 37.52 kB = 6.38 kB 6.38 kB
main.d.ts = 4.36 kB 4.36 kB = 1.09 kB 1.09 kB

Generated by 🚫 dangerJS against 329561d

@ikovic
Copy link
Collaborator

ikovic commented Jun 20, 2024

@jake-slashid I like the runtime change, but I feel it would still be well worth it to also prevent this during static checks - having a union type discriminated by the anonymousUsersEnabled property on the SlashIDProvider could filter out memory as an option when that flag is raised.

const STORAGE_TOKEN_KEY = "@slashid/USER_TOKEN";

const createStorage = (storageType: StorageOption) => {
const createStorage = (storageType: StorageOption, options: { anonymousUsersEnabled: boolean }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this must be handled one level above, as the SlashID context provider is the owner of this business logic.

@ikovic
Copy link
Collaborator

ikovic commented Jun 20, 2024

As far as testing goes, it seems as there is a chunk of business logic in the SlashIDProvider:

const slashId = new SlashID({
        oid,
        ...(environment && { environment }),
        ...(baseApiUrl && { baseURL: baseApiUrl }),
        ...(sdkUrl && { sdkURL: sdkUrl }),
        ...(typeof analyticsEnabled === "boolean" && { analyticsEnabled }),
      });
      const storage = createStorage(tokenStorage);

      storageRef.current = storage;
      sidRef.current = slashId;

IMO there is enough logic here that it could be wrapped in a domain function like createConfig that would take in the props sent to the SlashIDProvider and transform them to the required shape, applying validation where needed. Similar to what SlashIDOptions class is doing in the core SDK.

The function could be exported individually so we could test it easily with a unit test, no mocking needed.

What do you think?

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.

3 participants