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

adding isomorphic support #31

Closed
wants to merge 3 commits into from
Closed

adding isomorphic support #31

wants to merge 3 commits into from

Conversation

Jxck
Copy link

@Jxck Jxck commented Nov 7, 2014

I really know this spec is for browser.
but I believe make it isomorphic works fine for developer.
if I use this API on both browser and server.
it will help boost javascript isomorphic.
ex)

  • sharing logic in node.js.
  • running test on cli without heavy headless browser.

@josh
Copy link
Contributor

josh commented Nov 7, 2014

I think this polyfill is only going to be targeting browsers, not server side environments.

@matthew-andrews
Copy link
Contributor

@josh Would you still be against this if the node bits were put in their own file keeping the main polyfill clean of node code?

ie.

  • undo all changes to fetch.js (except the this/global thing)
  • updating package.json so the main pointed to a new file, say, node-fetch.js; and add a new key browser pointing to fetch.js ( for browserify ).
  • swapping bluebird for es6-promise
  • create a new file called, say node-fetch.js containing:-
require('es6-promise').polyfill();
global.XMLHttpRequest = require('xmlhttprequest').XMLHttpRequest;
require('./fetch');

Alternatively, I could put this stuff in a new repo that pulls your fetch in as a dependency (that new repo could also be published on npm, which would solve the problem for people who want isomorphic fetch without changing this repo?

return
}

var Promise = global.Promise;
if (typeof Promise === 'undefined') {
Promise = require('bluebird');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will mean that bluebird's code will always be added to this file when it's browserified and even delivered to browsers that already support Promises… This will make the compiled code a lot bigger — not sure you want to do it like this… I've written an alternative suggestion in the comments.

@josh
Copy link
Contributor

josh commented Nov 7, 2014

I think a node implementation ought to be a separate project. It'd probably
be better to implement direct to node http interface just like the node XHR
library itself.
On Fri, Nov 7, 2014 at 12:16 AM Matt Andrews notifications@github.com
wrote:

@josh https://github.com/josh Would you still be against this if the
node bits were put in their own file keeping the main polyfill clean of
node code?

ie.

  • undo all changes to fetch.js (except the this/global thing)
  • updating package.json so the main pointed to a new file, say,
    node-fetch.js; and add a new key for the `
  • swapping bluebird for es6-promise
  • create a new file called, say node-fetch.js containing:-

require('es6-promise').polyfill();global.XMLHttpRequest = require('xmlhttprequest');require('./fetch');

Alternatively, I could put this stuff in a new repo that pulls your fetch
in as a dependency (that new repo could also be published on npm, which
would solve the problem for people who want isomorphic fetch without
changing this repo?


Reply to this email directly or view it on GitHub
#31 (comment).


var XMLHttpRequest = global.XMLHttpRequest;
if (typeof XMLHttpRequest === 'undefined') {
XMLHttpRequest = require('xmlhttprequest').XMLHttpRequest;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I suspect this might bloat the client side code when compiled with browserify – you might be shipping an XMLHttpRequest polyfill to browsers… which I don't think would ever be used

@matthew-andrews
Copy link
Contributor

it'd probably be better to implement direct to node http interface

You don't like my 3 line solution? It's very maintainable :D

I do agree though that it would be best ( from a performance perspective ) if it were talking direct to node apis rather than through a polyfill designed for browser but I think if you're using this library performance is probably a less of a concern than being able to write reusable code (if you were worrying about performance you shouldn't use a promise based ajax library — you should use one that implements streams).

Given this is so small & quick I'll make this into a separate repo for now?

@Jxck
Copy link
Author

Jxck commented Nov 7, 2014

or smallest change with removing 'window' and inject with global this will help noder to use it in isomorphic way I think.

@matthew-andrews
Copy link
Contributor

@Jxck I'm having another problem – this doesn't look like it's the global object for a node file being 'required':-

Compare this:-

echo 'console.log(this === global);' > include-me.js && \
node -e "require('./include-me'); ";
// => false

With this:-

node -e "console.log(global === this);"
// => true

Am I doing something wrong?

@dgraham
Copy link
Contributor

dgraham commented Nov 7, 2014

If I'm understanding this properly, the request is for this web browser API polyfill be changed to work in a Node.js server runtime.

The Fetch specification is written with browser constraints in mind. As the spec evolves, so will the polyfill. It's going to, as strictly as possible for a polyfill, match the specification without concern for other possible runtimes.

Node.js already contains API for making HTTP requests. If there is a need to wrap that in a fetch-like API, it can be implemented in a separate project that can take full advantage of Node.js server features. Let's allow this implementation to optimize for browsers and another for Node.js.

@dgraham dgraham closed this Nov 7, 2014
@matthew-andrews
Copy link
Contributor

Sorry @dgraham & @josh, I really liked @Jxck's idea of being able to use the same code using Fetch API on the client and the server (if only for prototypes) so I made it into a separate thing here…

https://github.com/matthew-andrews/isomorphic-fetch

[Please don't suspend my GitHub account 😶]

@dgraham
Copy link
Contributor

dgraham commented Nov 7, 2014

@matthew-andrews
Copy link
Contributor

😇

@josh
Copy link
Contributor

josh commented Nov 7, 2014

nice!

@Jxck
Copy link
Author

Jxck commented Nov 8, 2014

@matthew-andrews in node, module.exports === exports === global this
in browser window === global this
so you can write isomorphic code for both env like this

// fetch.js
this.fetch = function() {};
<script src='fetch.js'>
<script>
window.fetch()
</script>
// node
var fetch = require('.fetch').fetch;
fetch();

and global in node already exists.
so global var in my pr wasn't good sorry.

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

Successfully merging this pull request may close these issues.

4 participants