microsoft / fluidframework Goto Github PK

node_modules@prague\container-loader\src\loader.ts takes options any, we should add an interface here ILoaderOptions, and then use interface augmentation in sub-classes to add optional properties to the options, so we get strong typing for options.

Docs are at 0.0.6; need to update them.

The unit tests in the repo all use mocha, which isn't in line with what the big app teams at Microsoft use. For reference OWA, ODSP, Office Online, SDX, Office UI Fabric React, and many others are using jest for unit tests. There are also big open source projects like Webpack and React that are using jest.

Here are related lines of code.
Note that the hack in OdspDocumentService.ts works for now as join session provides us tokens. That is going away and the app is responsible for fetching tokens. Fluid-fetcher is already doing it storage / vroom, but not for push.
BTW, can we cleanup driver to use same terminology? In some cases we use vroom token terminology, in others - storage token, there might be other terms used.

packages\tools\fluid-fetch\src\fluidFetchInit.ts
const getWebsocketTokenStub = () => Promise.resolve("");

packages\drivers\odsp-socket-storage\src\OdspDocumentService.ts

        // This is workaround for fluid-fetcher. Need to have better long term solution
        webSocketToken ? webSocketToken : websocketEndpoint.socketToken,

Looking through bug bash files, one had a lot of blobs, and specifically a lot of empty blobs.
There were a bunch of tables inserted (and deleted), and many had empty "tardis" & "header" blobs (for things like "cols", "rows", "matrix", etc.).

It would be great to optimize it, probably at driver level is the right place to do so.
That might also help with dedupping .attributes (specifically for maps), which are around 120 bytes today. Same for directory's attributes, which I saw in big numbers in same document (around 50).

This looks bad - we should put all the errors into telemetry.
Can you please follow up?
I'd do code review across all files related to summaries for these kinds of issues.
Thanks!

        doneP.then(
            () => {
                // In the future we will respawn the summarizer - for now we simply stop
                // this.computeSummarizer(this.connected)
                debug("summary generation complete");
            },
            (error) => {
                debug("summary generation error", error);
            });

From Tyler's feedback:

it seems like you can only store an UnboxedOper in a SparseMatrix, and UnboxedOper = undefined | boolean | number | string;

I think this constraint is arbitrary (due to historical ties to calc) and can be trivially relaxed to any JSON serializable type (see also #136)

Currently the merge tree and merge tree client expose a number of methods that are bound to specific segment types. These methods make less sense now that we are trying to generalize merge tree beyond text.

• remove initial text from merge tree and client constructors
• remove text specific methods from merge tree and client
• remove Item specific methods from merge tree and client

Open question, are marker segments available in all tree types? Or should they be abstracted away as well?

Currently the no-op acks from the client which are intended to be broadcast immediately (indicated by having some non-null contents) are sent immediately. An improvement would be to consolidate them to send a single one after a tick, and only if an outbound op wasn't already sent during that tick.

I have an initial implementation of this in a branch, but have not tested it yet.

I find it very frustrating to spend cycles on linting and formatting issues when a computer can do these automatically on save or precommit. For other repos, like office-ui-fabric-react, I never have to worry about the nit picky lint configurations like trailing commas and newlines since they have configured the tooling to do this automatically for us.

I'd consider this a blocking issue for going open source. The JS community expects this to work and I think this will be an unnecessary barrier to entry for open source contributions

My React-based Fluid component is not rendering in Edge since updating to release 0.9
In the latest server version, the "Fluid" clippy banner appears at the top of the page, but everything else is blank. On the previous version, the screen was simply blank.
In the Edge debug tools I can see that the page is loaded, but the "content" div is blank.

The issue reproduces with a new default sample React project (click counter) generated with "yo fluid".

Chrome renders as expected.
Dev server version is 3.9.1

I'm running Edge version 44.18362.267.0 and Chrome version 76.0.3809.132

We should either clear any references to this flag from code base, or allow individual cases to use it, but add a bunch of asserts into mergetree.ts validating that if blockUpdateMarkers === false, then there are no markers in that instance of merge tree.

Many other open source repositories, like office-ui-fabric-react support auto fixing of TS lint issues by vscode and all code formatting is handled by a tool like prettier.

This saves a lot of cycles for folks that work in multiple repos, each with different conventions.

Related microsoft/Prague#1381

I'm not sure we want to keep registers. They are currently not used heavily, are not snapshotted, never clean-up, and not tested well.

If we keep them merge tree should add a new op for copying text to a register, and we should remove the register property from remove op, cut then becomes a group op of copy, and remove.
It's probably ok to keep the register on insert for paste.

Currently due to change in join session response from spo, it no longer supply the storage and push tokens in join session. So because of that the spo route is broken.

Local test server doesn't support signals. In Scriptor-Pad (Local Fluid Document), when a signal is submitted, only the editor who sends the signal can receive it.
Scriptor will start to use signals for presence, and we need the local test server for testing.

If you attempt to create an instance of an Aqueduct component via IComponentContext.createComponent(), it will never be initialized.

This is due to code in SharedComponent.initialize() that skips initialization on the assumption that forge() will be called later.

            // If the runtime is existing we will execute the internal initialize. Otherwise the initialize
            // happens during the forge
            if (this.runtime.existing) {
                this.initializeP = this.initializeInternal();
            } else {
                this.initializeP = Promise.resolve();
            }

When trying to build Prague locally for the first time I run into problems, because there isn't any top-level readme on what's needed and how to do that. There is one deep in the Routelicious folder. I'd say it should be moved up and updated for people to see and be able to get started with working in the repo.

Can we remove requestWithRefresh()?
And replace it with getWithRetryForTokenRefresh if functionality is needed?
Thanks!

This stack is problematic:
at DeltaManagerProxy.emit (events.js:132)
at DeltaManager. (deltaManagerProxy.ts:23)
at DeltaManager.emit (events.js:151)
at DeltaQueue. (deltaManager.ts:185)
at DeltaQueue.emit (events.js:151)
at callback (deltaQueue.ts:137)
at DeltaQueue.worker (deltaManager.ts:176)
at DeltaQueue.processDeltas (deltaQueue.ts:151)

Note EventEmitter implementation - this screws up error processing - we should not throw from here (but we do because of DeltaManagerProxy):

// If there is no 'error' event listener then throw.
if (doError) {
var er;
if (args.length > 0)
er = args[0];
if (er instanceof Error) {
// Note: The comments on the throw lines are intentional, they show
// up in Node's output if this results in an unhandled exception.
throw er; // Unhandled 'error' event
}

Currently the summarizer cannot listen for summary acks or nacks to make better decisions about when to summarize. Sometimes the summarizer will send two consecutive summary ops with the same parent, which causes the second one to be (correctly) rejected by the server.

The summarizer currently has 3 methods for summarizing:

1. On receiving an op, after a certain number of ops
2. On receiving an op, after a certain amount of time
3. On a timer set for a certain amount of time after the last op was received (idle timer)

This logic should be cleaned up to better align the above conditions with each other (when any of the 3 are met, it should reset all 3 if it is not already). And further, we should have another time threshold to wait for an ack/nack before trying to summarize again.

We have a lot of opportunity to improve quality of life for Fluid contributors by making better use of workspace settings and possibly recommended extensions in vscode.

Many MergeTree operations refer to the position at the end of the sequence. For example, consider appending to the end of a sequence:

"0123"
     ^-- Append = insert @4

In order to use LocalReferences to track caret positions, selection ranges, etc. we need the ability to create LocalReference to the position after the last segment in the MergeTree (@4 above).

Today, the LocalReference can only references positions 0..3 in the above example. This limitation has impacted all user of LocalReferences that I'm aware of:

• FlowView does not use LocalReferences. Instead, it observes ops to update the caret/selection offsets manually.
• SharedStringInterval users can only include the last position in a sequence if they treat 'end' as inclusive instead of exclusive (#1761). This has been source of numerous off-by-one errors in Table-Slice / Tablero. (I found another one yesterday.)
• WebFlow used to attempt to preserve an extra marker at the end of the document. If this is accidentally deleted, the last position in the document can no longer be edited.
• WebFlow now works around this by crafting a LocalReference that points to sentinel segment that is never inserted into the MergeTree. (See Flow-Document addLocalRef, etc.)

We are trying to load a container with a null version as a query parameter to skip snapshots since they won't exist in this scenario. This request also sets some request headers.

The end result is that this line of code
https://github.com/microsoft/FluidFramework/blob/master/packages/loader/container-loader/src/loader.ts#L263
sets the version back to undefined.

Perhaps, we should only have one way of setting the version?

From Tyler's Soduku feedback:

Mistake #1: Trying to store objects in a DDS that are not safely JSON round-trippable

This is approximately the solution:

export type JsonPrimitive = undefined | boolean | number | string;
export type JsonObject = { [index: string]: JsonPrimitive | JsonArray | JsonObject };
export type JsonArray = (JsonPrimitive | JsonObject)[];
export type Json = JsonPrimitive | JsonArray | JsonObject;

As I've mentioned before (#3145), we should also call Object.freeze(..) on these objects.

Property update overrides existing property on a segment (with only exception of local properties).
Basically Merge tree implements "last one wins" merge resolution for annotates.

This is problematic for snapshots, because "header" (state of sequence at MSN) may be generated before last annotate op, but it will contain properties of that future annotate op.
I'm not 100% sure, but that feels like can cause trouble.
Couple scenarios to verify:

1. What happens with local ops that are not acked before we generate snapshot?
2. What happens when we mix and match different rules about applying properties? I see "rewrite" mode in extend<>() in properties.ts.

Maybe it's not an issue, but I want to make sure we validate it's not an issue.

Example of the error (from logs):

0: "connect_document_error"
1: "activityLimitReached (Client): Request Id: bdb1049f-605f-0000-c05f-b1b7de6d6333. Status Code: 429. Response Body: {"error":{"innerError":{"code":"throttledRequest"},"code":"activityLimitReached","message":"The request has been throttled","retryAfterSeconds":120}}"

Need to respect retryAfterSeconds

we ask for /opStream/latest then ask for deltas, then ask for /version and finally ask for /snapshot.

Tasks

• Add a shared ESLint config
• Add eslint config files to all lerna-managed packages
• Add eslint and necessary plugins to each project
• Configure tslint-to-eslint plugin
• Configure the missing imports plugin
• Configure the React plugin
• Remove tslint-to-eslint plugin and tslint for each project
• Switch CI to run eslint tasks instead of tslint
• Remove tslint NPM tasks

Our DDSes allow you to modify objects that are set in the DDS. These modifications do not trigger new DDS value sets and cause bugs because the local state of the developer is different from remote clients.

Example

const myObject = {
  id: 1;
}

sharedMap.set("my-object-id", myObject);

myObject.id = 2;

In the above example locally myObject would be have an id of 2 but the DDS and all connect clients would have an id of 1. This is because setting the id on the object does not trigger an update to the SharedMap. These are difficult bugs for developer to find.

Solution

To prevent this we can freeze any object set using JavaScript Object.freeze(object) within the DDS.

Work

DDSes that need this change

• SharedCell
• SharedMap
• SharedDirectory
• SharedString - property bag

The current merge tree snapshot includes all zamboni'd segments. And then plays tardis'd messages between the min sequence number and the snapshot sequence number. This requires us to create, load and replay these tardis'd operations. Load can be simplified and storage decreased by including segments that haven't been zamboni'd in the snapshot.

Dedup url resolver code for odsp/routerlicious. Currently it is present at multiple places like gateway, hosts, praguedump, odsp utils.

Many popular JS OSS projects (including React) are using prettier to automatically format their code. It would be great if this repo could use prettier to enforce consistent code style.

Previously the sequence would become active after only loading the header.

But a recent change waited on body load prior to giving access https://github.com/microsoft/Prague/blob/12c12a3bea16181fd478ea22dd8f04a01b327338/packages/runtime/sequence/src/sequence.ts#L612.

For documents that can't delay load this is not problematic. But for those that can, like flow-view, it's a big perf hit (seconds).

This was simpler when prepare existed because the runtime would await the sequence being fully loaded before inbounding new ops (it would still allow outbound). In the updated world the sequence will need to be able to handle ops for segments it has not yet loaded.

packages\agents\spellchecker\src\spellchecker.ts

Attempting to compile *.wasm files loaded via the webpack dev server fails. Compilation fails because Webpack-Component-Loader routes all requests not ending in '.js(.map)' to a static HTML page:

app.get(/(.*(?<!\.js(\.map)?))$/i, (req, res) => fluid(req, res, baseDir, options));

Locally modifying the above regex to also exclude *.wasm files works around the issue.

For context, I hit this while using an OSS parser framework/generator that produces *.wasm files to syntax highlight WebFlow documents.

Any chance you can see if we deal with reconnections properly in summarize?

1. Do we destroy summarizing container on lost connection?
   Note - createSummarizer() has "fluid-reconnect": false, which seems correct, assuming the rest works
2. Do we always recreate summarizer on same client on reconnect if there were (and there are) no other clients connected?

Any chance that we can add telemetry (erorr) when there is no summurizer in a document and given client is connected?

Thanks!

Why is Docker a requirement for building the repo? It seems very unnecessary, and makes it harder for people to contribute.

Examples of things to consolidate:

• promiseResolveQueue
• processLocalMessage(..)
• submit(..)
• onDisconnect()

ISegment is very coupled to BaseSegment, infact an ISegment that doesn't inherit from BaseSegment probably won't be valid. This seems like a bad situation, and i'd like to trim down ISegment so it is less complex, and host it off a new leaf node type that handles much of what is in BaseSegment.

This would also open up the possibility of allowing user defined Segment types, as potentially sequence could run in a mode where the segment data is never deserialized, it's basically just json hanging off the leaf. The snapshotter could then run in this mode, and not care what kind of segments are in the sequence.

On caveat here would be the ability of the snapshotter to run Zamboni, but I thinks this could be resolved to some extent, it just may not be able to Zamboni as deeply, as the Zamboni constraints would need to be derived from invariants or accessed from the segments as simple json properties. Currently the only segment type Zamboni constraints are:

• Segment that can never be appended; marker, external
• If types match, and length is less than some max: text, and subsequence

All other constraints come from the BaseSegment properties that would move to the new leaf node type

On a Windows 10 machine, I've cloned the repo and ran npm install without having Docker Desktop running.

I was greeted by error messages like these, which included a password:

> root@ postinstall:r11s D:\FluidFramework
> cd server/routerlicious && npm run registryAuth


> root@ registryAuth D:\FluidFramework\server\routerlicious
> docker login -u prague -p A_PASSWORD_WAS_HERE prague.azurecr.io

WARNING! Using --password via the CLI is insecure. Use --password-stdin.
error during connect: Post http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.40/auth: open //./pipe/docker_engine: The system cannot find the file specified. In the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running.
npm ERR! code ELIFECYCLE
npm ERR! errno 1                                                                                                        
npm ERR! root@ registryAuth: `docker login -u prague -p A_PASSWORD_WAS_HERE prague.azurecr.io`             
npm ERR! Exit status 1                                                                                                  

After starting Docker, install succeeds.
It still shows the password in the log - do we care about it?

We have an NPM install post-install task that does some docker related tasks. There is some subset of developers that don't work on the server pieces and have no need to set up docker on their machines, so it is pretty frustrating to have this error message after every npm install:

'docker' is not recognized as an internal or external command,
operable program or batch file.