State of SockJS in Meteor

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.

8 Likes

wow, copy forked!!! It seems there’re lots of things I don’t know about Meteor :sweat_smile:
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?

I believe this is the culprit in question.

2 Likes

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

Sorry, I forgot to add the most important link:

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

/Per

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

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

@coagmano is this something that needs more attention?

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:

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:

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



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?

9 Likes

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

2 Likes

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

2 Likes

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

3 Likes

Thanks a lot for the archaeology Fred :slight_smile:

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…

5 Likes

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

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

3 Likes

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

2 Likes

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?

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

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 :slight_smile: )?

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?).

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

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?

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