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

Convert to CJS, compile to UMD with Browserify #1

Open
wants to merge 4 commits into
base: add-nodejs-support
Choose a base branch
from

Conversation

markdalgleish
Copy link

This commit standardises the code between browser and Node. Branching logic to handle the different
environments is no longer necessary.

Thanks to Browserify we can use Node's http module in the browser instead of using XHR directly, so 'http.js' has simply become 'makeRequest.js' and now exports what was previously the 'makeRequestForNode' function.

BenSayers and others added 3 commits January 6, 2015 22:51
This commit standardises the code between browser
and Node. Branching logic to handle the different
environments is no longer necessary. Thanks to
Browserify we can use Node's http module in
the browser instead of using XHR directly, so
'http.js' has simply become 'makeRequest.js' and
now exports what was previously the
'makeRequestForNode' function
@markdalgleish
Copy link
Author

What are your thoughts on this commit? Obviously it's a somewhat large change, but it should significantly simplify the process of maintaining support for both Node and the browser.

@BenSayers
Copy link
Owner

Thanks @markdalgleish, I'll have a look at this tonight.

@BenSayers
Copy link
Owner

I like what you've done here and agree it will be easier to maintain if we can abstract away the differences between the the environments we want to support. Managing dependencies using the node style require is also a welcome addition.

My biggest concern with this change is how much additional code wound up in the end result - it went from ~300 lines to ~7000 lines. I know that this is a testing tool and that the file size doesn't matter very much but I'd like to discuss if we can do something about this before accepting this change. What are your thoughts on this?

I haven't used browserify before so I'll spend some time reading up about it so I can be less ignorant.

@markdalgleish
Copy link
Author

I noticed the extra code also—this is because all the code for the http module has been included for the browser—but like you pointed out, I'd argue that this shouldn't necessarily hold up this pull request since file size isn't much of an issue for a testing tool.

In the meantime I'll look into optimising the build so that the client code remains lightweight.

@airmanx86
Copy link

I also agree with Ben. Mark, I believe using http module is an over kill which come with extra dependencies that the client code base does not need. A simple abstraction layer might be just good enough.

@markdalgleish
Copy link
Author

I agree that the http module is overkill for the client, but I think it's the lesser of two evils right now. I'd argue that it's more important that the Node/browser branching is eliminated to ease the maintenance burden of keeping the two environments in sync.

If this project was designed for end users I would 100% agree that this would be unacceptable, but for a testing tool I think it's okay.

@airmanx86
Copy link

https://github.com/cujojs/rest
We can draw inspiration from this project, in terms of it being built into two outputs - a browser version (with bower support) and node version (with npm support). Note that I am not talking about the REST client but the build output.

Quoting from the change log:
"separate browser and node main modules, browser consumers should switch their main module from 'rest/rest' to 'rest/browser'. This allows tools such as browerify and webpack to more intelligently reason about the module structure."

@markdalgleish
Copy link
Author

I've updated the code to use an XMLHttpRequest shim for Node (xhr2) so that the resulting browser bundle is nice and lean. It now comes in at 276 lines.

The 'browser' property is used in package.json
so that Browserify doesn't include xhr2 in the
browser bundle.
@markdalgleish
Copy link
Author

I've also sent a PR to xhr2 (pwnall/node-xhr2#14) so that the browser version is provided automatically, which would allow shims/xhr2.js to be deleted.

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