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

Remove unnecessary dependency on server-services-client package #17478

Merged
merged 18 commits into from Sep 29, 2023

Conversation

jikim-msft
Copy link
Contributor

Description

5659

This PR replaces @fluidframework/server-services-client to @fluid-internal/client-utils in the @fluidframework/tinylicious-driver.

@github-actions github-actions bot added area: driver Driver related issues dependencies Pull requests that update a dependency file public api change Changes to a public API base: main PRs targeted against main branch labels Sep 26, 2023
@jikim-msft jikim-msft marked this pull request as ready for review September 26, 2023 19:53
@jikim-msft jikim-msft requested review from msfluid-bot and a team as code owners September 26, 2023 19:53
@anthony-murphy
Copy link
Contributor

did we consider reducing our dependency over all here. I think in general we could just replace this with uuid. they are a bit less nice, but dropping the dependency overall is probably worth it.

@alexvy86
Copy link
Contributor

alexvy86 commented Sep 26, 2023

did we consider reducing our dependency over all here. I think in general we could just replace this with uuid. they are a bit less nice, but dropping the dependency overall is probably worth it.

Good point... the uses I saw were basically to generate fake user names, where we already use uuid() for a fake user id. Using the first two segments of the same uuid as <first> <last> seems like it would be fine. @jikim-msft once you're done finding all the places where we can make this update, let's see how those packages are using getRandomName(). If that's the only usage pattern (generate a random name for a fake user), I like Tony's suggestion to just remove the function entirely and generate the fake names from the corresponding uuid.

@anthony-murphy
Copy link
Contributor

did we consider reducing our dependency over all here. I think in general we could just replace this with uuid. they are a bit less nice, but dropping the dependency overall is probably worth it.

Good point... the uses I saw were basically to generate fake user names, where we already use uuid() for a fake user id. Using the first two segments of the same uuid as " " seems like it would be fine. @jikim-msft once you're done finding all the places where we can make this update, let's see how those packages are using getRandomName(). If that's the only usage pattern (generate a random name for a fake user), I like Tony's suggestion to just remove the function entirely and generate the fake names from the corresponding uuid.

it should be all test/demo/example. a more involved change would be to allow name injections, but default to uuid, and then directly depend on silly name in the webpack fluid loader, which is what our examples generally use

@jikim-msft jikim-msft changed the title Remove server dependency on t9s-driver Remove server dependency on client-release group Sep 26, 2023
@jikim-msft
Copy link
Contributor Author

jikim-msft commented Sep 26, 2023

@alexvy86

Files listed below depend on getRandomName() only for the purpose of assigning random names to the fake user:

  • packages/drivers/local-driver/src/auth.ts
  • packages/drivers/tinylicious-driver/src/insecureTinyliciousTokenProvider.ts
  • packages/tools/webpack-fluid-loader/src/getDocumentServiceFactory.ts

I think we can replace the name to have a first few segments of uuid.

@@ -49,9 +48,12 @@ export function generateToken(
}

export function generateUser(): IUser {
const userId = uuid();
const userName = userId.match(/^([\da-f]{8})-([\da-f]{4})/); // userName takes the first two segments of the userId.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes the intention a bit clearer, can we update them all?

Suggested change
const userName = userId.match(/^([\da-f]{8})-([\da-f]{4})/); // userName takes the first two segments of the userId.
const userName = userId.match(/^([\da-f]{8})-([\da-f]{4})/); // Just use the first two segments of the (fake) userId as a fake name

packages/tools/webpack-fluid-loader/package.json Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the public api change Changes to a public API label Sep 27, 2023
@tylerbutler
Copy link
Member

The title of the pr is misleading. Maybe "remove unnecessary dependency on server packages"?

@jikim-msft jikim-msft changed the title Remove server dependency on client-release group Remove server dependency on server package Sep 27, 2023
@tylerbutler tylerbutler changed the title Remove server dependency on server package Remove unnecessary dependency on server package Sep 27, 2023
@tylerbutler tylerbutler changed the title Remove unnecessary dependency on server package Remove unnecessary dependency on server-services-client package Sep 27, 2023
@jikim-msft jikim-msft closed this Sep 28, 2023
@github-actions github-actions bot removed area: driver Driver related issues dependencies Pull requests that update a dependency file labels Sep 28, 2023
@jikim-msft jikim-msft reopened this Sep 28, 2023
@github-actions github-actions bot added area: driver Driver related issues dependencies Pull requests that update a dependency file labels Sep 28, 2023
@jikim-msft jikim-msft merged commit 23324ff into microsoft:main Sep 29, 2023
28 checks passed
jikim-msft added a commit that referenced this pull request Sep 29, 2023
#### Description

While removing dependency on `server-services-client` package
(#17478), found that the
`local-driver` and `webpack-fluid-loader` packages do not have
`types/uuid` package which enables type definition for uuid.
@jikim-msft jikim-msft deleted the t9s/remove-dependency branch September 29, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: driver Driver related issues base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants