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: Use nanonid/non-secure instead of cuid #1912

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap added the core Infrastructure work going on behind the scenes label Apr 19, 2023
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

👍 We can start with this, it will take care of the yarn warning.

In direction 13 mode cuid is probably overkill for what we use it for. Most usage we can probably replace with simpler methods:

  • nodeId: we can probably replace by an id based on an incrementing integer, these don't benefit anymore from being globally unique. In fact they are regenerated every time the dom is loaded from disk
  • page file id: we can probably replace by a name generator. Could be more user friendly as well, until we decide to change the routing to be file name based
  • monaco editor, we can probably also replace it by an id based on an integer.

For later though!

@bharatkashyap
Copy link
Member Author

@Janpot https://app.circleci.com/pipelines/github/mui/mui-toolpad/6991/workflows/e354df79-c205-4539-a273-61fc4dfb4023/jobs/30387 Is it apparent to you why this test is failing - I can't see why this fails

@Janpot
Copy link
Member

Janpot commented Apr 19, 2023

Does it pass locally?

@bharatkashyap
Copy link
Member Author

Does it pass locally?

No, fails with the same mysterious stack trace

@@ -235,7 +237,7 @@ function assertIsType<T extends AppDomNode>(node: AppDomNode, type: T['type']):
}

function createId(): NodeId {
Copy link
Member

@Janpot Janpot Apr 19, 2023

Choose a reason for hiding this comment

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

cuid2 API seems to be designed to init only once.

import { init } from '@paralleldrive/cuid2';

const createIdInternal = init({ length: 7 })

export function createId(): NodeId {
  return createIdInternal() as NodeId;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This still fails on yarn test with the same error as in the CI

Copy link
Member

Choose a reason for hiding this comment

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

yes, I wasn't proposing this as a fix for that, just as a suggestion to use the lib in a more idiomatic way.

Let's just move to nanoid/non-secure instead of cuid2. That's more than suitable for our needs and won't have these issues

@@ -1077,7 +1077,7 @@ async function initToolpadFolder(root: string) {
apiVersion: 'v1',
kind: 'page',
spec: {
id: cuid.slug(),
id: cuid2({ length: appDom.SLUG_LENGTH })(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
id: cuid2({ length: appDom.SLUG_LENGTH })(),
id: appDom.createId(),

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2023
@bharatkashyap bharatkashyap changed the title Fix: Upgrade to cuid2 Fix: Use nanonid/non-secure instead of cuid Apr 24, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2023
@bharatkashyap bharatkashyap merged commit 7cc2496 into mui:master Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update cuid to paralleldrive/cuid2
2 participants