mondora / asteroid

An alternative client for a Meteor backend
MIT License
734 stars 101 forks source link

Added websocket import #122

Closed holmrenser closed 7 years ago

holmrenser commented 7 years ago

This pull request adds the import of the WebSocket package (https://www.npmjs.com/package/ws) to be able to run the asteroid package on node.

See #121

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 12622f7faabb5084a68bd0c95b4382d8634dd28b on holmrenser:master into 7192311afc731ceffed7d11b6b005e764fcfbd7c on mondora:master.

Falieson commented 7 years ago

I have pushed my own version of this PR to falieson/asteroid until this is merged which can be installed with npm i falieson/asteroid and used just like asteroid.

The difference between this and the PR is only my removal of /lib from the .gitignore and pushing the babel compiled version up.

Falieson commented 7 years ago

@holmrenser don't know why your changes would cause this but when I use my compiled version of this PR (npm i falieson/asteroid) I get CORS errors that I can't resolve by adding to browser-policy to Meteor's server/main.js

Falieson commented 7 years ago

This should not be merged, I use this PR for testing but it causes a CORS issue in my frontend. @holmrenser did you overcome the CORS websocket issue somehow?

holmrenser commented 7 years ago

This pull request was an error on my side. Asteroid allows you to specify the exact WebSocket implementation you want:

const asteroid = require('asteroid');
const WebSocket = require('ws');

const Connection = asteroid.createClass()

const portal = new Connection({
    endpoint: 'ws://localhost:3000/websocket',
    SocketConstructor: WebSocket // <-------------- HERE
})

You should only use the above code when using node on the server. In the browser asteroid uses the browser WebSocket variable so you do not need to do anything there.

121

https://stackoverflow.com/a/45697156/6573438

Falieson commented 7 years ago

Thank you very much for the explanation. @holmrenser What if we added some node environment variables to conditionally import and add the socketConstructor. There are some standard ones that I believe will map to most people's setups, I just haven't done the research to know the right way of doing this yet. This would solve my jest issue for asteroid. Unfortunately, it doesn't seem like the maintainer has had a chance to catch up on PRs. I've started to use a local npm registry to get around unreleased versions of packages.