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

build(client-azure-client): cleanup dependencies #23650

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

jason-ha
Copy link
Contributor

  • Removed [unused] production dependencies: map and runtime-utils
  • Import SharedMap and SharedTree from fluid-framework.
    • SharedTree is different between tree and fluid-framework exports. The test code that is using SharedTree wants to use the fluid-framework @public version but was using the @legacy version from tree. (Really tree should expose both, but it is understandable why it is the way it is.)
    • For best patterns and consistency, SharedMap should be pulled from fluid-framework instead of map.
    • Then neither map nor tree are required dependencies and are removed.
  • Removed dev dependencies: aqueduct

- Removed [unused] production dependencies: `map` and `runtime-utils`
- Import `SharedMap` and `SharedTree` from `fluid-framework`.
   - `SharedTree` is different between `tree` and `fluid-framework` exports. The test code that is using `SharedTree` wants to use the `fluid-framework` `@public` version but was using the `@legacy` version from `tree`. (Really `tree` should expose both, but it is understandable why it is the way it is.)
   - For best patterns and consistency, `SharedMap` should be pulled from `fluid-framework` instead of `map`.
   - Then neither `map` nor `tree` are required dependencies and are removed.
- Removed dev dependencies: aqueduct
@jason-ha jason-ha requested review from Copilot, alexvy86, ChumpChief, Josmithr and sonalideshpandemsft and removed request for Copilot January 24, 2025 21:38
@github-actions github-actions bot added dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Jan 24, 2025
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

The fluid-framework dependency scares me a little bit but I guess it's probably fine?

@@ -128,6 +124,7 @@
"cross-env": "^7.0.3",
"eslint": "~8.55.0",
"eslint-config-prettier": "~9.0.0",
"fluid-framework": "workspace:~",
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 is probably fine since it's a dev dependency and only for use in testing (to make it more authentic to what a customer would do)?

I do think we should continue to avoid taking non-dev dependencies on fluid-framework in our own packages to keep our layering simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think we should continue to avoid taking non-dev dependencies on fluid-framework in our own packages to keep our layering simple.

I agree, but hereby present what seems like a legit exception to the rule 😄 : #23647. That's an e2e tests package though, which arguably should be a @fluid-internal one (I'd say @fluid-private but I suspect we do need it published internally due to how we run those tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh agree, was mostly thinking about product (shipping) packages. Anywhere where we are acting "as a customer would" (e2e tests, examples, etc.) the dependency seems fine to me.

@jason-ha jason-ha merged commit 768cc1e into microsoft:main Jan 24, 2025
37 checks passed
@jason-ha jason-ha deleted the azure-client-dep-cleanup branch January 24, 2025 22:29
jason-ha added a commit to jason-ha/FluidFramework that referenced this pull request Jan 24, 2025
that appeared in microsoft#23650 but unrelated to the dependency changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants