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

make dataloader work in the browser #112

Closed
wants to merge 2 commits into from

Conversation

jeffreyyoung
Copy link

Not sure if this is the best way to implement this feature. But I thought it'd be nice to be able to use DataLoader in the browser

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ab46ff5 on jeffreyyoung:master into dc3f0cd on facebook:master.

@timsuchanek
Copy link
Member

This would be really awesome as client-side requests could be cached/batched

@Vanuan
Copy link

Vanuan commented Nov 15, 2017

@leebyron any chance to merge it?
What's missing from this pull request?
Do you prefer to use something like this https://github.com/michaelrhodes/iso-next-tick ?

@Vanuan
Copy link

Vanuan commented Nov 15, 2017

Hm... It looks like I it can be easily solved by some change in webpack config

@Compulsed
Copy link

It would be awesome for this to work in the browser, in the mean time does anyone know any alternatives?

@jeffreyyoung
Copy link
Author

@Compulsed an alternative if you want to use the library in the browser now, is just fork the library, and replace process.nextTick with setTimeout in this file https://github.com/facebook/dataloader/blob/master/src/index.js#L212

@Vanuan
Copy link

Vanuan commented Nov 23, 2017

Doesn't webpack already implement process.nextTick?

process.nextTick(fn);
} else {
setTimeout(fn);
}
Copy link

Choose a reason for hiding this comment

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

Personally, I'd simply inline the check for node here. The overhead of a complete Environment class seems rather heavy to me. Most runtimes I can think of at the moment support setTimeout so we'd only have to check for node.

Something like this would suffice in my opinion. That way the code and performance impact is guaranteed to be trivial:

resolvedPromise.then(() => {
  if (process !== undefined && process.nextTick !== undefined) {
    process.nextTick(fn);
  } else {
    setTimeout(fn);
  }
});

(Note though that I'm not a contributor to dataloader. The core members probably have their reasons)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah That makes sense to me. I initially thought to do it that way, but wasn't quite sure how to get 100% test coverage. Do you know how i could get this logic process !== undefined && process.nextTick !== undefined to return false in the tests?

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.

None yet

6 participants