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

Bind the step function to the active domain (if present) #197

Merged
merged 3 commits into from
May 26, 2015

Conversation

mehcode
Copy link
Contributor

@mehcode mehcode commented May 4, 2015

This allows for libraries and applications 3 or 4 layers up to be able to use generators in a domain and have the domain context be persisted.


In order to accept your pull request, we need you to submit a CLA. You only need to do this once, so if you've done this for another Facebook open source project, you're good to go. If you are submitting a pull request for the first time, just let us know that you have completed the CLA and we can cross-check with your GitHub username.

I've completed the CLA.

@@ -121,8 +119,15 @@
} else {
Promise.resolve(info.value).then(callNext, callThrow);
}
};

if (process.domain) {
Copy link
Contributor

Choose a reason for hiding this comment

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

process wont always be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about the browser environment; fixed

@@ -121,8 +119,15 @@
} else {
Promise.resolve(info.value).then(callNext, callThrow);
}
};

if (process && process.domain) {
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 still throw in browsers since process wont be defined. You'll need to do typeof process !== "undefined".

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not ensure that process is declared. It only ensures that process does not have a falsy value. It will still throw if it is not declared.typeof is pretty much the only way to go here.

@fkling
Copy link
Contributor

fkling commented May 4, 2015

Not sure why our comments are marked as outdated. Either way, process && process.domain won't guard against undeclared process.

@mehcode
Copy link
Contributor Author

mehcode commented May 4, 2015

That makes sense. I should have known that. Fixed.

@mehcode
Copy link
Contributor Author

mehcode commented May 12, 2015

Is there something else I need to do to get this merged?

@mehcode
Copy link
Contributor Author

mehcode commented May 26, 2015

@benjamn ^

Not trying to be pushy but my team is stuck on this fork because our project uses domains and regenerator (through babel) and they'd like to get rid of the npm-shrinkwrap.json file.

benjamn added a commit that referenced this pull request May 26, 2015
Bind the step function to the active domain (if present).
@benjamn benjamn merged commit ab10563 into facebook:master May 26, 2015
@benjamn
Copy link
Collaborator

benjamn commented May 26, 2015

Published to NPM as v0.8.27: http://npmjs.org/package/regenerator

@benjamn
Copy link
Collaborator

benjamn commented Jul 21, 2017

@Andarist At the time this pull request was merged, there were still lots of Node projects that hadn't stopped using process.domain (even though it was going to be deprecated). Relevant documentation and discussions from back then:

It's probably safe to remove this now, if you want to submit a PR!

@Andarist
Copy link
Contributor

wow, ive commented directly on the commit, had no idea my comment would land in the issue thread, nice! :)

@benjamn thanks for the answer, I would love to send a PR, although I would include it as part of the #302 if you decide to give it a green light, if not - im gonna send a separate, small PR to remove those lines

@mehcode
Copy link
Contributor Author

mehcode commented Jul 22, 2017

We're no longer using domains anymore, yeah. Don't have the time to send a PR but we won't notice it missing.

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

Successfully merging this pull request may close these issues.

6 participants