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

Cleanup of runtime code #6872

Merged
merged 5 commits into from
Sep 13, 2021
Merged

Conversation

kenluck2001
Copy link
Contributor

@kenluck2001 kenluck2001 commented Jul 26, 2021

This PR refactored the source code to remove these concepts from repo: #4691

TreeEntry.Commit
"commit"
ISnapshotTree.commits

Not sure if if #4683 is fully covered too

@ghost
Copy link

ghost commented Jul 26, 2021

CLA assistant check
All CLA requirements met.

@github-actions github-actions bot added area: driver Driver related issues area: loader Loader related issues area: runtime Runtime related issues area: server Server related issues (routerlicious) breaking change This PR or issue would introduce a breaking change labels Jul 26, 2021
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jul 26, 2021

@fluidframework/base-host: No change
Metric NameBaseline SizeCompare SizeSize Diff
main.js 187.08 KB 187.08 KB No change
Total Size 214.67 KB 214.67 KB No change
@fluid-example/bundle-size-tests: -409 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
container.js 162.13 KB 161.97 KB -172 Bytes
map.js 44.74 KB 44.66 KB -79 Bytes
matrix.js 143.54 KB 143.47 KB -79 Bytes
odspDriver.js 175.31 KB 175.31 KB No change
odspPrefetchSnapshot.js 38.98 KB 38.98 KB No change
sharedString.js 165.12 KB 165.04 KB -79 Bytes
Total Size 762.5 KB 762.1 KB -409 Bytes

Baseline commit: 2ca9937

Generated by 🚫 dangerJS against c270260

@kenluck2001 kenluck2001 changed the title Cleanup of runtime code #4691 Cleanup of runtime code Jul 26, 2021
@tylerbutler
Copy link
Member

@kenluck2001 Thank you for this PR and #6876 ! There's an internal event at Microsoft this week, so you may not see any feedback on these PRs until next week. Just wanted to let you know we're not ignoring these. :)

@vladsud
Copy link
Contributor

vladsud commented Jul 28, 2021

@jatgarg, can you please help with CR here? Specifically, assess if cleanup can be done across server/client in one go (i.e. I'd think we want to build repos linked together to prove correctness).
I'm also not sure about connection to ISnapshotTree.commits and ITreeEntry.type === "commit" (see for example buildHierarchy). Do we need to remove ISnapshotTree.commits as well to ensure that we are not missing anything and cleanup indeed is possible, mechanical and has no impact on existing functionality?
Some context - Kenneth is helping here as part of his FHL project, he is new to FF.

@tanviraumi - please glance as well, I'm surprised a bit it's so easy :) Does R11S need to retain any of the logic related to commits? From quickly glancing through code, it feels like we do not use commits at all nowadays and we can do cleanup (including wiping out commits from ISnapshotTree).

@kenluck2001 - some context for you: We have 3 separate builds as part of repo - common, server, client. They depend on each other through versioning mechanism. I.e. changes you are making here on the server side are not consumed in same build by client build - client keeps using old definitions, until new server packages are published and client bumps dependencies (versions in package.json) to consume them. So while build succeeded, it does not prove that all commit references (on client side) are removed - we may find surprises down the road when client consumes these new server bits.. Thus I'm a bit worried this is not complete fix, and a bit more involved validation might be required. We have certain ways to simplify such validation (by linking builds together), and that's required to prove to ourselves that indeed direction is right, but it is a bit involved operation

@curtisman
Copy link
Member

Ping @jatgarg @tanviraumi

@tanviraumi
Copy link
Contributor

FYI @znewton

@jatgarg
Copy link
Contributor

jatgarg commented Sep 7, 2021

I am trying to test these changes on my local box.

@kenluck2001 kenluck2001 requested a review from a team as a code owner September 7, 2021 23:16
@github-actions github-actions bot removed request for a team and arinwt September 7, 2021 23:16
@github-actions github-actions bot added public api change Changes to a public API and removed breaking change This PR or issue would introduce a breaking change labels Sep 7, 2021
@jatgarg
Copy link
Contributor

jatgarg commented Sep 7, 2021

Added some changes. The previous changes also look fine to me. Ran snapshot tests with these changes. So approving. We can merge once @znewton also approves this.

@jatgarg jatgarg merged commit 8c84ff7 into microsoft:main Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: driver Driver related issues area: loader Loader related issues area: runtime Runtime related issues area: server Server related issues (routerlicious) community-contribution public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants