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

Mixin RequestBase functions from a prototype #1088

Merged
merged 1 commit into from
Oct 23, 2016

Conversation

ahelmberger
Copy link
Contributor

This PR is an alternative solution to #1076

  • Make RequestBase a constructor function
  • Make it mixin its prototype functions when called as a function
  • Unifies mixin approach with Emitter
  • Fixes global scope issues in module bundlers like rollup.js

References #1076

See also:
rollup/rollup#1007
rollup/rollup#1023
rollup/rollup-plugin-commonjs#127

* Make RequestBase a constructor function
* Make it mixin its prototype functions when called as a function
* Unifies mixin approach with Emitter
* Fixes global scope issues in module bundlers like rollup.js
@ahelmberger
Copy link
Contributor Author

Red build on TravisCI doesn't seem to be a code issue.

@kornelski
Copy link
Contributor

Thank you for the PR. That looks like a good way to solve it.

I wonder if we need to bump semver-major for this? (will this impact supertest, plugins or mocking?) WDYT @focusaurus?

@focusaurus
Copy link
Contributor

It might be worth getting some explicit coordination from the supertest maintainers on this branch before merging. I believe their mechanism to integrate with superagent will continue to work but I think it would be worthwhile to test. Looks like @mikelax is the most recent active maintainer. Perhaps I could try to pull down the supertest repo, integrate it with this branch of superagent, and see if their unit tests still pass.

@focusaurus
Copy link
Contributor

I was able to:

  • clone supertest
  • npm install, run tests, they pass
  • rm -rf ./node_modules/superagent && npm i 'ahelmberger/superagent.git#mixin-requestbase-prototype'
  • supertest unit tests still pass

So that's a good sign.

@focusaurus
Copy link
Contributor

Pragmatically I don't think there's much harm done in treating this as semver major even if in retrospect we find out nothing important broke. That's probably better than semver minor that unexpectedly breaks some integration, so I wouldn't sweat it too much.

@kornelski
Copy link
Contributor

kornelski commented Oct 23, 2016

Cool. Let's do that as semver-major. It's now (unrelated to this issue) also a good time to drop Node 0.10 support (official support has ended), which might be seen as semver-major too.

@kornelski kornelski merged commit 1cb3870 into ladjs:master Oct 23, 2016
@mindrones
Copy link

mindrones commented Oct 29, 2016

Hi, this patch is in master but not in the latest release: would it be possible to release so that superagent can be used in projects built with Rollup? Thanks!

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.

None yet

4 participants