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 PropertyDDS packages which are not depended on by PropertyDDS #22326

Merged
merged 6 commits into from
Sep 5, 2024

Conversation

CraigMacomber
Copy link
Contributor

@CraigMacomber CraigMacomber commented Aug 28, 2024

Description

Deletes some packages from experimental/PropertyDDS.

The DDS and its dependencies are kept, but the rest of its packages are not.

Delete the following published experimental packages:

  • @fluid-experimental/property-shared-tree-interop
  • @fluid-experimental/property-binder
  • @fluid-experimental/property-proxy
  • @fluid-experimental/property-inspector-table

Delete the following private packages:

  • @fluid-example/property-inspector
  • @fluid-example/schemas

Changes made by deleting the packages, then running:

pnpm i --no-frozen-lockfile
pnpm layer-check --md .

and manually adding a changeset.

Breaking Changes

These packages will no longer be published:

  • @fluid-experimental/property-shared-tree-interop
  • @fluid-experimental/property-binder
  • @fluid-experimental/property-proxy
  • @fluid-experimental/property-inspector-table

Reviewer Guidance

The review process is outlined on this wiki page.

@github-actions github-actions bot added base: main PRs targeted against main branch area: dds: propertydds changeset-present dependencies Pull requests that update a dependency file labels Aug 28, 2024
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Aug 28, 2024

@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 461.15 KB 461.18 KB +35 Bytes
azureClient.js 559.19 KB 559.24 KB +49 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 261.99 KB 262 KB +14 Bytes
fluidFramework.js 400.11 KB 400.12 KB +14 Bytes
loader.js 134.26 KB 134.28 KB +14 Bytes
map.js 42.39 KB 42.39 KB +7 Bytes
matrix.js 146.56 KB 146.56 KB +7 Bytes
odspClient.js 526.47 KB 526.52 KB +49 Bytes
odspDriver.js 97.72 KB 97.74 KB +21 Bytes
odspPrefetchSnapshot.js 42.78 KB 42.79 KB +14 Bytes
sharedString.js 163.26 KB 163.26 KB +7 Bytes
sharedTree.js 390.62 KB 390.63 KB +7 Bytes
Total Size 3.3 MB 3.3 MB +245 Bytes

Baseline commit: 84b5c4e

Generated by 🚫 dangerJS against cba5f8d

@CraigMacomber CraigMacomber marked this pull request as ready for review September 3, 2024 16:19
@CraigMacomber CraigMacomber requested a review from a team as a code owner September 3, 2024 16:19
@tylerbutler
Copy link
Member

Obligatory question: were you able to check with partners to see if this change would impact them?

Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

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

Approving for docs team.

@CraigMacomber
Copy link
Contributor Author

CraigMacomber commented Sep 3, 2024

Obligatory question: were you able to check with partners to see if this change would impact them?

Last Wednesday in our sync they were asked if they had any reason not to do this (and they didn't), and I'm giving them a week find any reasons they could not think of at the time. Should I receive no opposition by tomorrow by tomorrow, I think it makes sense to go ahead with this removal. None of these packages have been actively maintained, and I confirmed the two main customers have their own forks/alternatives to property inspector table which the independently maintain and would not be impacted by this one being removed. As for the other packages, no on expressed any opinions yet, which means no reason to keep them.

We can of course restore these packages is needed, though at that point I'd suggest who ever needed them actively maintain the package, and prefer they be restored to a different repository unless there is a good reason they need to be in this one.

Update changeset

Co-authored-by: Tyler Butler <[email protected]>
@CraigMacomber
Copy link
Contributor Author

CraigMacomber commented Sep 3, 2024

Autodesk has requested a 2-3 day timeline extension to finish reviewing their use of these packages. moving the expected date for proceeding with this back to ~ Friday.

Edit: Got the go-ahead from Autodesk. Will merge.

@CraigMacomber CraigMacomber enabled auto-merge (squash) September 5, 2024 15:31
Copy link
Contributor

github-actions bot commented Sep 5, 2024

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  378641 links
    2984 destination URLs
       2 URLs ignored
       0 warnings
       0 errors


@CraigMacomber CraigMacomber merged commit 8db660e into microsoft:main Sep 5, 2024
33 checks passed
@CraigMacomber CraigMacomber deleted the trimPropertyDDS branch September 5, 2024 16:25
Abe27342 added a commit that referenced this pull request Jan 6, 2025
## Description

Removes property-query-service in LTS and update the lock file. This
package was removed from main in #20052.
Also removed several other PropertyDDS auxiliary packages which were
removed from main in #22326 and #20119:

- @fluid-experimental/property-query
- @fluid-experimental/property-binder
- @fluid-experimental/property-proxy
- @fluid-experimental/property-inspector-table
- @fluid-example/property-inspector
- @fluid-example/schemas
- @fluid-experimental/partial-checkout

This resolves a CVE for `[email protected]` that was pulled in as a
transitive dependency of this PropertyDDS package.


[AB#25462](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/25462)

---------

Co-authored-by: Abram Sanderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: propertydds base: main PRs targeted against main branch changeset-present dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants