State of SockJS in Meteor

permb · February 17, 2020, 11:01am

Background:
Sockjs is used for all websocket connections in meteor. It promises to provide a http fallback in cases where native websockets don’t work.

The current sockjs implementation was copy-forked (the library source code was copied into meteor) in 2012 using version 0.3.4 of sockjs and after that the real sockjs library has evolved to version 1.4.0 while the copied code also received changes that were the reason to fork the code way back.

There are known bugs like this one and no one has appeared yet that show a deep understanding of the code and could help fix it.

So I wonder what would be a good way forward. This is such a crucial part of Meteor and also hard to test.

harry97 · February 17, 2020, 11:50am

wow, copy forked!!! It seems there’re lots of things I don’t know about Meteor
It’s actually quite humbling to know about this issues since it wasn’t mentioned during the roadmap forum thread afaik.

Some side question though, wouldn’t updating it affect the current DDP implementation? Which would in turn cause a chain reaction effect of ruining many packages that are based on it?

Also, just for clarification purposes is this the fork you’re referring to?

copleykj · February 17, 2020, 12:00pm

I believe this is the culprit in question.

jkuester · February 17, 2020, 12:51pm

I think this could be part of Transition as much as possible to NPM in the roadmap.

permb · February 17, 2020, 1:19pm

Sorry, I forgot to add the most important link:

github.com

meteor/meteor/blob/devel/packages/socket-stream-client/sockjs-0.3.4.js

// XXX METEOR changes in <METEOR>

/* SockJS client, version 0.3.4, http://sockjs.org, MIT License

Copyright (c) 2011-2012 VMware, Inc.

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

This file has been truncated. show original

That is the “copy-forked” file I was referring to.

/Per

jkuester · February 17, 2020, 1:32pm

But checking the package.js shows the 0.3.4 only be used for legacy:

github.com

meteor/meteor/blob/devel/packages/socket-stream-client/package.js#L18


Npm.depends({
  "faye-websocket": "0.11.1",
  "permessage-deflate": "0.1.6"
});


Package.onUse(function(api) {
  api.use("ecmascript");
  api.use("modern-browsers");
  api.use("retry"); // TODO Try to remove this.


  api.addFiles("sockjs-0.3.4.js", "legacy");
  api.mainModule("browser.js", "client", { lazy: true });


  api.addFiles("server.js", "server");
  api.mainModule("node.js", "server", { lazy: true });
});


Package.onTest(function(api) {
  api.use("underscore");
  api.use("ecmascript");
  api.use("tinytest");

Edit: I jsut saw in the browser.js file that @coagmano was working on this.

@coagmano is this something that needs more attention?

coagmano · February 18, 2020, 12:14am

Yeah this does need more attention.

Lets look at the history:
There was an attempt a while ago to only load SockJS when it was needed for compatibility:

github.com/meteor/meteor

Avoid loading SockJS in browsers known not to need it.

meteor:devel ← meteor:sockjs-polyfill

opened 05:06PM - 12 Nov 17 UTC

benjamn

+236 -33

Supersedes #9316, in which @stubailo demonstrated that `SockJS` can be straightf…orwardly eliminated if native WebSocket support is assumed. Native WebSocket support is close to universal now (94% of all clients according to https://caniuse.com/#feat=websockets), which means we can remove ~30KB of legacy code from the client bundle that we send to most web browsers. To achieve this flexibility, the `sockjs-0.3.4.js` script is injected as a standalone `<script>` tag into the `<head>` of the document, but only if the user agent string of the HTTP request fails to convince the `sockjs-shim` package that the browser has native WebSocket support. When Meteor is running in production, a minified version of that script is used instead. Not only is this loading strategy a bit faster for older browsers, because the `SockJS` script begins loading earlier, and is independent from the normal JS bundle, it also removes the cost of loading SockJS completely for more recent browsers. Though it's tempting to keep hacking the SockJS script to make it smaller, we should avoid spending too much time on that, since it only benefits a small portion of users whose devices are likely to have poor performance for lots of other reasons. In other words, I think we should keep [Amdahl's Law](https://en.wikipedia.org/wiki/Amdahl%27s_law) in mind.

Which conditionally used server-render to add a script tag with SockJS using the sockjs-shim package

And then with the introduction of modern/legacy bundling, SockJS is always loaded for legacy browsers, and for modern ones, it waits for an error before loading it with a dynamic import.

Collapse sockjs-shim into socket-stream-client with web.browser.legacy.
Try loading SockJS dynamically after native WebSocket errors.

And then after 1.7.0.1, people started getting errors related to this and SockJS was statically imported for modern bundles as well:

github.com/meteor/meteor

Statically import SockJS in both modern and legacy bundles.

meteor:devel ← meteor:statically-import-SockJS-again

opened 05:56PM - 12 Jun 18 UTC

benjamn

+6 -1

Not including `SockJS` in the modern JS bundle was a nice bundle size savings (2…8KB before gzip), but `SockJS` works better than a native `WebSocket` for clients that are stuck in unusual networking situations, and the fallback of using dynamic `import()` to load `SockJS` when the native `WebSocket` failed was much slower than simply including `SockJS` in the bundle and using it from the start. Moreover, the new `meteor create --minimal` starter app does not use `socket-stream-client` (nor DDP), so going back to including `SockJS` in both the modern and the legacy bundles should have no impact on the minimal modern bundle size. If you want to continue using a native `WebSocket` instead of `SockJS`, you can always pin the older version of the socket-stream-client package: ```sh meteor add socket-stream-client@0.2.1 ``` Once this change is published, the workaround I suggested to @p3pp8 here will no longer be necessary: https://github.com/meteor/meteor/pull/9917#issuecomment-395798377

Later, there was a discussion about how the use of dynamic-import forces you to allow the unsafe-eval browser policy, and about being able to remove dynamic-import from meteor apps if you wanted to.
The only core package that required dynamic-import was socket-stream-client for the case where a modern browser failed to connect to a websocket, SockJS would be loaded and used instead.
Because #9985 had made that import static, the dynamic-import was redundant and removed to allow folks to remove dynamic-import and enforce no unsafe-eval

github.com/meteor/meteor

Remove redundant dynamic-import from socket-stream-client

meteor:devel ← coagmano:remove-dynamic-import-from-socket-stream-client

opened 12:49AM - 16 Oct 19 UTC

coagmano

+1 -11

Currently it's very difficult to remove `dynamic-import` from a Meteor app (#107…04) because of strict dependencies from `ecmascript` and `socket-stream-client` (aka `ddp`). This also means it's very difficult to disable `unsafe-eval` in CSP as `dynamic-import` sets `BrowserPolicy.allowEval()`. Looking at `socket-stream-client`, the only dynamic import is rendered redundant by #9985 and so this PR cleans up the `import` call and dependency. Maybe the optimization that #9985 reverts will come back, in which case it would be nice if SockJs could be conditionally bundled or `import()`ed based on whether the `dynamic-import` package is installed

In the end, we’re back where we started, with a copy-forked SockJS 0.3.4 loaded for all browsers

The ideal would be that native sockets are used where possible and fall-back to SockJS’s socket or http polling approach, with SockJS always loaded for legacy browsers

Looking at the errors that caused Ben to import SockJS statically for modern again, I think it was unrelated to SockJS and instead was a bug in dynamic-import not setting the correct url for fetching modules. I recognise that error because we ran into it a bit around that time as well, especially when connecting to another meteor instance across a network.

It might be possible to combine all the past approaches, and use dynamic-import if the package is available, and otherwise using server-render for modern only as a backup?

brianmulhall · February 18, 2020, 12:55am

this topic got me thinking about how important it is that meteor have some people volunteering to maintain all the tests and to help write new test cases. so developers can make changes and feel confident they arent introducing regressive behavior

coagmano · February 18, 2020, 2:30am

While undoubtably true, I don’t know how you would test websocket / SockJS compat between old and new browsers, in a variety of network situations from good, to spotty, to proxied

brianmulhall · February 18, 2020, 2:38am

i agree, this is a difficult thing to test.

But in general I feel like alot of effort has been made to cover meteor with automated tests and now that I read through the source in my free time i would like to be in a position to help maintain that test coverage in the future

permb · February 18, 2020, 12:36pm

Thanks a lot for the archaeology Fred

I’m not sure that the current dynamic-import package should be used for this since sadly it requires relaxing CSP to allow unsafe-eval.

But as has been said testing a matrix of scenarios would be tricky…

jkuester · February 18, 2020, 5:34pm

@permb where does ist allow unsafe eval? Can you elaborate on that? I didn’t dig into this so much yet.

permb · February 18, 2020, 7:51pm

The implementation for dynamic-import can be found in the meteor repo, and there you can find both the “eval” calls to load a module data into executable code and also the setup where the package adds unsafe eval to the csp statements.

It would be quite tricky to polyfill this, but now that Microsoft is moving Edge to Chromium there is hope for native (modern) support for this feature.

Making it possible to exclude dynamic-import from the ecmascript package would be a start towards being able to provide a custom implementation, eg a polyfill that loads packages from a static location

coagmano · February 18, 2020, 11:45pm

Meteor is unlikely to use a native implementation as it currently has a lot of extra features to cache modules in indexDB and only fetch exactly the modules missing for that specific client. ie. perfect module splitting

Maybe the WCG will eventually reach feature parity with module-maps, but I don’t think this feature set is in the works yet.

Which is why my suggestion is to add a weak dependency, so it uses it if it’s already installed, and fallback to always loading SockJS when it’s not

permb · February 19, 2020, 5:38am

Sounds reasonable. What would be the best implementation strategy for this? I’ve never worked on the core pieces of Meteor.

Is it possible to fork just the stream package and add it as an override somewhere to isolate the changes into a separate package while it is being worked on?

coagmano · February 20, 2020, 1:19am

Yep!
Any package, either core, non-core or atmosphere, can be copied into the /packages/ folder of your app and it will use your local version instead. Extremely useful when you just want to make one change to an atmosphere package!

Although, editing in a local clone of the Meteor repo has the advantage of testing against Meteor’s test suite. You can test it with your app by setting the METEOR_PACKAGE_DIRS environment variable to use the packages folder of your Meteor clone

permb · February 20, 2020, 7:57am

Great - although I was thinking beyond local development. Is there any way to publish a forked version of a core Meteor package and make your builds use it, i.e. a “package override” (with lots of warning messages about how you’re on your own if you do this )?

In my instance, I would want to build a deployable package from my circle-ci build server to test any changes in my staging/test AWS environment and then it would be much nicer to just do that from a branch where I have specified a forked socket-stream-client build in .meteor/ packages|versions.

I haven’t seen any note of such an ability but it seems it would be useful instead of forking the entire meteor project and publish all of meteor to atmosphere (or however people usually approach this - is there a way to publish to your own atmosphere repo?).

coagmano · February 20, 2020, 10:12am

Yes if it’s in your apps /packages/ folder, it will use that in all contexts. So even when it’s built in CI!

permb · February 20, 2020, 12:07pm

Oh I get it - thanks. What’s then the best way to redistribute the patched package so others can test it too?

One way could be to add in a fork of meteor as a git submodule, have it .meteorignore’d and then symlink the package in question to each project’s packages directory.

Or is there a supported way to publish a forked core meteor package and get it used by different people?

cereal · February 20, 2020, 3:28pm

Anyone who wants to test it could clone your git repo into their own packages directory.