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

Shift responsibility to stringify message content from Loader to runtime layer #11819

Merged
merged 1 commit into from Sep 6, 2022

Conversation

vladsud
Copy link
Contributor

@vladsud vladsud commented Sep 3, 2022

This (in future) will help us avoid double strigification for scenarios like chunked ops, compressed ops.

…ime layer

This (in future) will help us avoid double strigification for scenarios like chunked ops, compressed ops.
@github-actions github-actions bot added area: loader Loader related issues area: runtime Runtime related issues public api change Changes to a public API base: main PRs targeted against main branch labels Sep 3, 2022
@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: +364 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 391.54 KB 391.64 KB +100 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 190.91 KB 191.01 KB +100 Bytes
loader.js 151.74 KB 151.9 KB +164 Bytes
map.js 42.63 KB 42.63 KB No change
matrix.js 131.29 KB 131.29 KB No change
odspDriver.js 150.24 KB 150.24 KB No change
odspPrefetchSnapshot.js 38.4 KB 38.4 KB No change
sharedString.js 152.53 KB 152.53 KB No change
Total Size 1.25 MB 1.25 MB +364 Bytes

Baseline commit: dcf9599

Generated by 🚫 dangerJS against d10ffc7

@vladsud vladsud marked this pull request as ready for review September 3, 2022 01:43
@vladsud vladsud requested review from msfluid-bot and a team as code owners September 3, 2022 01:43
return this.submitMessage(
type,
// back-compat: ADO #1385: pass content as-is in future.
typeof contents === "string" ? contents : JSON.stringify(contents),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do the same thing for signals? It will keep the layers consistent across our message types

Copy link
Contributor

Choose a reason for hiding this comment

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

slightly unrelated, but we also process signals in the delta manger, specifically we json parse them, and loose data, like the ref seq, which prevent us for leveraging ref seq on signals in the runtime. maybe this should be a parallel work stream to get the treatment between the two symmetrical, and ideally basically transparent to the loader layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but do not want to make these PRs more complex.
https://dev.azure.com/fluidframework/internal/_workitems/edit/1841

@vladsud vladsud merged commit 2a04b0f into microsoft:main Sep 6, 2022
@vladsud vladsud deleted the OpProcessing5_sending branch September 6, 2022 20:25
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

@@ -80,6 +80,9 @@
"RemovedEnumDeclaration_BindState": {
"forwardCompat": false,
"backCompat": false
},
"InterfaceDeclaration_IContainerContext": {
"forwardCompat": false
Copy link
Member

Choose a reason for hiding this comment

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

@vladsud - I don't think we can make any changes like this (breaking so-called forwardCompat or backCompat) for IContainerContext in main. In this case, an old implementation does not reflect the new interface. I know you add a runtime check, but this is actually causing a type compatibility problem in Bohemia codebase. I think we can work around it but just bringing to your attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right. I assumed that interface would be used only internally, but it's clearly wrong assumption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: loader Loader related issues area: runtime Runtime related issues base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants