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

Add browserify support #71

Merged
merged 1 commit into from
Feb 21, 2015
Merged

Conversation

deathcap
Copy link
Member

@deathcap deathcap commented Jan 7, 2014

I wanted to use node-minecraft-protocol in the web browser via browserify, but it fails because of the ursa native library dependency:

Error: module "../bin/ursaNative" not found from "node-minecraft-protocol/node_modules/ursa/lib/ursa.js"

So I added a browser-specific module entry point, which does not include any ursa-dependent functionality. This means auth/crypto/etc is unavailable in this environment, but you can still encode and decode packets.

(@roblabla has been working on replacing ursa with Forge, pure JavaScript: https://github.com/roblabla/node-minecraft-protocol/tree/feature-purejsrsa-forge - which could also solve this unbrowserifyable natives problem. But at least for my purposes, only including lib/protocol when browserified was sufficient. Didn't need any of the decryption functionality.)

@mappum
Copy link
Contributor

mappum commented Jan 7, 2014

I don't know why you would want to do this, browsers don't provide access to raw TCP sockets so there would be no way to use the protocol (without piping it through something else like a Websocket proxy, in which case you wouldn't need to do the serialization in the browser).

You would be best off making a Minecraft server plugin that accepts Websocket connections, or a proxy that takes a Websocket connection and pipes it to a real protocol connection.

@roblabla
Copy link
Member

roblabla commented Jan 7, 2014

Just saying, while using forge RSA works, performance goes down by quite a bit. It's okay for the client, where RSA is needed only once, but for the server, it's probably a terrible idea ^^. That's why I never submitted PRs for it.

@deathcap
Copy link
Member Author

deathcap commented Jan 7, 2014

@mappum I did (https://github.com/deathcap/wsmc/), and I was originally decoding the packets on the server-side before sending to the web client (JSON encoding the fields from parsePacket, sending as text over the WS). However for large binary packets like map_chunk_bulk this proved inefficient, so I switched to sending the raw MC packets over binary websockets and decoding with node-minecraft-protocol in the browser.

@roblabla Yeah, and in the browser running bulk AES decryption would probably be a bad idea too. I just handle all the encryption/decryption/handshaking in the proxy, so the browser only has to take care of the actual packet payload parsing.

@andrewrk
Copy link
Member

andrewrk commented Jan 7, 2014

what do you think about this idea:

Separate node-minecraft-protocol into 2 modules - one which only does packet serialization and deserialization. Another module would depend on this one (this one would be the one that retains the name minecraft-protocol) and would have the same API and would depend on the other module. It would do the handshake/login, encryption, and socket connection handling.

The lower level protocol module would work in browserify since it would have no dependencies. In fact this is something that I've suggested before.

@deathcap
Copy link
Member Author

deathcap commented Jan 7, 2014

👍

@deathcap
Copy link
Member Author

deathcap commented Jan 8, 2014

@superjoe30 I'll close this PR, agreed having separate modules is a much better and cleaner solution. Will definitely switch mcwebchat/voxel-clientmc to use the zero-dependency protocol parsing module if/when it is released.

@deathcap deathcap closed this Jan 8, 2014
@deathcap deathcap reopened this Apr 30, 2014
@deathcap
Copy link
Member Author

@andrewrk @roblabla Reopening for reconsideration, I was updating https://github.com/deathcap/wsmc and ran into this again. It seems like a fairly clean & easy solution IMHO to add the "browser": "browser.js" tag to package.json, only including lib/protocol.js. This PR still merges cleanly.

Wouldn't be opposed to splitting up the node-minecraft-protocol module into one with only protocol de/serialization, and another with all the other code — though that would add some more complexity; this change would be more expedient. What does everyone think?

deathcap added a commit to deathcap/wsmc that referenced this pull request Apr 30, 2014
Updated as of upstream 2014/04/14 PrismarineJS/node-minecraft-protocol@3e2f703
But depends on two other changes:
PrismarineJS/node-minecraft-protocol#83 Emit a 'raw' event for all raw buffers
PrismarineJS/node-minecraft-protocol#71 Add browserify support
@roblabla
Copy link
Member

I'm a bit iffy with the idea of splitting the project, IMO it adds more maintenance overhead than is necessary. However, this change feels more like a hack than an actual fix.

I believe it is possible to define modules-overrides for the browser, so incompatible node modules can be replaced with compatible ones. (https://github.com/substack/node-browserify#browser-field) I could try to make my forge fork API-compatible with ursa, and then all we'd have to do is tell browserify to use forge instead of ursa. Then, you would be able to use the whole API (and if you only want protocol.js, that's fine).

What do you think ?

@deathcap
Copy link
Member Author

deathcap commented May 1, 2014

@roblabla sounds like that would work alright, too

@roblabla
Copy link
Member

roblabla commented May 1, 2014

Just saying, I'll work on this next week. I'm a bit very busy right now, got a project to hand in next monday, and I have about two weeks worth of work to do on it XD

@deathcap
Copy link
Member Author

Looking into this again, still interested in this feature, updates since:

  • ursa is now maintained again (transferred ownership)
  • node-minecraft-protocol falls back to node-rsa if ursa is unavailable (cool)

unfortunately, even with ursa listed in optionalDependencies in the package.json, it fails to browserify:

200    3ms     1.17KB  /bundle.js ➞ ./node_modules/watchify ./index.coffee -d --debug
200   87ms      1.2KB  /crosshair4.png
200   87ms     7.08KB  /logo-white.png
404   10ms        12B  /favicon.ico
200  280ms       777B  /
Error: Cannot find module '../bin/ursaNative' from '/Users/admin/games/voxeljs/voxpopuli/node_modules/voxel-clientmc/node_modules/mineflayer/node_modules/minecraft-protocol/node_modules/ursa/lib'
  at /Users/admin/games/voxeljs/voxpopuli/node_modules/browserify/node_modules/browser-resolve/node_modules/resolve/lib/async.js:43:25
  at load (/Users/admin/games/voxeljs/voxpopuli/node_modules/browserify/node_modules/browser-resolve/node_modules/resolve/lib/async.js:99:43)
  at /Users/admin/games/voxeljs/voxpopuli/node_modules/browserify/node_modules/browser-resolve/node_modules/resolve/lib/async.js:105:22
  at /Users/admin/games/voxeljs/voxpopuli/node_modules/browserify/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47
  at FSReqWrap.oncomplete (fs.js:99:15)

but since node-rsa is now available as a replacement for ursa, and browserify supports module substitution for the browser in package.json, tried this:

  "browser": {
    "ursa": "node-rsa"
  },

of course, node-rsa is not API-compatible with ursa so this doesn't work.. (pull out your rsa-wrap.js into a new module, to allow browser-based substitution?). Additionally the superagent module has trouble with loading mime, attempts to call `fs.readFileSync(file, 'ascii')' which is not available in browserify (and not covered by brfs - it only can replace fixed file paths), not sure what is going on there since superagent is advertised as supporting both Node.js and the browser (with the same API).

The original change in this pull request (only exposing lib/protocol.js in the browser, not pulling in any other dependencies) still works. Could this be merged in the meantime until the other features of node-minecraft-protocol (yggdrasil authentication, using superagent; protocol encryption, using ursa; etc.) are made to work the browser (if ever)? I only really am interested in the packet decoding/encoding.

(Tested against the packets-1.8 branch)

@roblabla
Copy link
Member

Shouldn't browserify ignore optionalDependencies if they fail to install ? This strikes me as a weird bug. But anyway, I'll move my rsa-wrapper in an npm module. ursa-purejs or something. I should've done that from the start.

I looked at superagent's source and couldn't find the offending line. Do you have the stacktrace? I think you might want to open an issue on their side.

Do note that the raw packet support in packets-1.8 branch is... well, completely broken. The packet format changed quite a lot in 1.8 (and might change again), and writeraw is very version-dependant.

I'll merge this in the meantime, but I hope to find a better solution eventually. I don't like adding a browser field in the package.json because we don't actually support the whole library inside a browser, just a tiny fraction of it.

roblabla added a commit that referenced this pull request Feb 21, 2015
Expose only protocol.js when used with browserify.
@roblabla roblabla merged commit 39fe1b5 into PrismarineJS:master Feb 21, 2015
@deathcap
Copy link
Member Author

Thanks!

(about the yggdrasil/superagent browser issue, posted more details in https://gist.github.com/deathcap/898bec7260a0b0dbf5cd, but probably not worth fixing, since XHR will not be able to contact the Mojang auth servers, anyway, due to cross-origin restrictions - and the only way to fix this, without Mojang enabling CORS on their auth servers, which I doubt they will, is to use a CORS proxy, which introduces security issues since a third-party has to pass through the user credentials. for browser auth, a different solution is needed - in wsmc, I'm experimenting with having the user login with the official client (authenticating to Mojang's servers as usual, online mode), then get a token to use for browser-based login, to solve these problems.)

@roblabla
Copy link
Member

It might be worth it asking grum about oauth. He said he wanted to add it a while ago... and then poof, nothing happened since.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants