feat: refactored to support node/browser targets using babel #1423

niftylettuce · 2018-10-01T22:26:28Z

this should resolve tons of issues, but namely these ones:

#1418, #1365, #1421, #1390, #1340, #1276, #1269

niftylettuce · 2018-10-01T22:31:23Z

@kornelski I also suggest that we use np and release to publish new versions moving forward

niftylettuce · 2018-10-01T22:32:11Z

There's an issue right now with eslint-config-xo-lass requiring Node v8.3+, I am fixing this, one moment.

ladjs/superagent#1423

niftylettuce · 2018-10-01T22:34:29Z

As an aside, if you use np, be sure to use it with np --no-yarn since yarn publish does not have OTP (two-factor auth) support yet.

niftylettuce · 2018-10-01T22:39:30Z

Tests should pass now, I think.

niftylettuce · 2018-10-01T23:56:24Z

I doubt that we can use this package, but I made it anyways https://github.com/niftylettuce/spdy-or-http2. Not sure of the differences or if there'd be a conflict. The only reason why tests fail is because http2 is not available in Node v6.x. The package I linked spdy-or-http2 may fix this issue, but again, I'm uncertain. In the meanwhile I've removed Node v6.x from being supported.

codecov-io · 2018-10-02T00:01:14Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a0e6a75). Click here to learn what that means.
The diff coverage is 92.3%.

@@            Coverage Diff            @@
##             master    #1423   +/-   ##
=========================================
  Coverage          ?   94.64%           
=========================================
  Files             ?       10           
  Lines             ?      766           
  Branches          ?        0           
=========================================
  Hits              ?      725           
  Misses            ?       41           
  Partials          ?        0

Impacted Files	Coverage Δ
src/node/parsers/text.js	`100% <ø> (ø)`
src/node/parsers/index.js	`100% <ø> (ø)`
src/node/parsers/image.js	`100% <ø> (ø)`
src/node/parsers/urlencoded.js	`90% <ø> (ø)`
src/node/unzip.js	`100% <100%> (ø)`
src/node/parsers/json.js	`100% <100%> (ø)`
src/node/response.js	`95.45% <50%> (ø)`
src/node/agent.js	`90.47% <87.5%> (ø)`
src/node/request.js	`93.87% <91.66%> (ø)`
src/node/http2wrapper.js	`96.61% <96.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0e6a75...bf8b9a2. Read the comment docs.

niftylettuce · 2018-10-02T00:03:35Z

✅ all tests are now passing btw

niftylettuce · 2018-10-02T00:06:43Z

I also had updated the docs too, you can view a preview at https://github.com/niftylettuce/superagent.

kornelski · 2018-10-03T02:33:48Z

That's a huge PR with many unrelated changes. It'll take me a while to review it.

niftylettuce · 2018-10-03T04:01:02Z

Thanks @kornelski, if I can answer any questions please let me know. This is blocking for me right now and I published @ladjs/superagent@5.0.0 in the meanwhile (you could also use this package for testing or to see what the NPM node_modules folder would look like, e.g. mkdir test && npm init && npm install @ladjs/superagent && cd node_modules/@ladjs/superagent && ls).

I didn't change any core code other than running it through the linter / xo / prettier with --fix command. I also just tested it in the browser myself using the jsdelivr package and it works great https://jsfiddle.net/t82ysrh7/4/.

Currently there are a ton of code quality issues with the existing codebase, such as using != vs !== and not using Object.hasOwnProperty.call. I did not want to make these changes without first getting this PR resolved. If you look at my branch you'll notice an extensive list of eslint rules that I've turned off due so that the linting tests will pass for now. See https://github.com/niftylettuce/superagent/blob/master/package.json#L205-L250 for this list.

The reason the PR is so huge is because I used https://lass.js.org, and therefore moved everything from lib to src folder (src folder gets built to lib using Babel with .babelrc Node/Browser targets thanks to @babel/preset-env.

larron · 2018-12-07T15:01:25Z

@niftylettuce Thanks for this, I was also blocked on the same issue with having ES6 slip into a final build. Your package @ladjs/superagent works perfectly.

kornelski · 2019-01-02T23:38:39Z

Hi!

Sorry, there are too many unrelated changes. I can't accept this PR. I don't have time to review 20000 lines of changes.

It doesn't matter that the changes are just from a code formatter, I can't accept code changes I haven't reviewed, and basically everything everywhere has changed.

I'm still keen to improve compilation for browsers, but just that. Please leave out changes of quoting style or moving commas and semicolons around.

niftylettuce · 2019-01-10T22:30:52Z

This will also fix #1437

I will release this soon, thank you folks for adding me as a contributor!

MatthewHerbst

Thanks for your work on this, especially making a browser compatible version.

Instead of switching to Yarn, please just start committing the package-lock.json from npm

MatthewHerbst · 2019-01-10T22:39:33Z

.babelrc

+    ["@babel/env", {
+      "targets": {
+        "node": "8.8.1",
+        "browsers": [ "> 1%", "last 2 versions", "ie >= 9" ]


Nit: is there a way to use the .browserlistrc file here so we don't have duplicated config?

There is not

MatthewHerbst · 2019-01-10T22:41:11Z

.gitignore

Nit: while not a change you made, package-lock.json should absolutely be being committed to the codebase.

MatthewHerbst · 2019-01-10T22:43:39Z

.npmignore

@@ -5,3 +5,5 @@ examples
 lib-cov
 coverage.html
 bower.json
+.nyc_output
+coverage


Nit: I don't think this file isn't being used because we already specify the files directive in package.json.

I had a scenario which happened multiple times (and I reported to NPM) where despite files and being in .gitignore, NPM tarball contained these folders, so it was just a safe guard.

MatthewHerbst · 2019-01-10T22:44:26Z

.remarkignore

@@ -0,0 +1,3 @@
+CONTRIBUTING.md


Does this repo use Remark?

It is now using it (via my PR), I have to update the contributors block still, it's pretty cool - gets read from contributors in package.json and automatically added to README.md in the # Contributors section.

MatthewHerbst · 2019-01-10T22:50:11Z

src/node/unzip.js

@@ -58,9 +58,9 @@ exports.unzip = (req, res) => {
  // override `on` to capture data listeners
  const _on = res.on;
  res.on = function(type, fn) {
-    if ('data' == type || 'end' == type) {
+    if (type == 'data' || type == 'end') {


Nit: use ===

I left most of the core logic as-is, this was from prior code. If you see I turned a TON of ESLint rules off so the tests would pass... there were so many existing issues with the old codebase.

MatthewHerbst · 2019-01-10T22:54:26Z

src/response-base.js

+  this.serverError = type == 5;
+  this.error = type == 4 || type == 5 ? this.toError() : false;
+
+  // sugar


Nit: use ===. use Number.parseInt(status, 10) if status is a string

Same as #1423 (comment), what we have to do is fix this everywhere by turning off the rules one by one and fixing lint errors. I didn't want to do a daunting commit with tons of changes everywhere, my first step was getting browser support done.

MatthewHerbst · 2019-01-10T22:58:09Z

package.json

+    "type": "git",
+    "url": "https://github.com/visionmedia/superagent"
+  },
+  "scripts": {


Nit: these should be near the top of the file

I use fixpack which organizes them differently, I think there might be a way to configure the order somehow with fixpack. Is there a better alternative than using fixpack?

It seems like what I'm looking for should be the default behavior: From the fixpack docs:

It will re-write your package.json file as follows:

name first

description second

version third

author fourth

all other keys in alphabetical order

dependencies and devDependencies sorted alphabetically

newline at the end of the file

It seems you could create a .fixpackrc file to manage the configuration.

Personally I just manage package.json files manually (save for npm install runs)

MatthewHerbst · 2019-01-10T22:59:38Z

package.json

+    "test-coverage": "npm run build && npm run lint && npm run nyc"
+  },
+  "unpkg": "dist/superagent.min.js",
+  "xo": {


This looks like an ESLint config. Can we use a .eslintrc file instead please? This really clutters this file

MatthewHerbst · 2019-01-10T23:00:17Z

src/client.js

    return new exports.Request('GET', method).end(url);
  }

  // url first
-  if (1 == arguments.length) {
+  if (arguments.length == 1) {


Nit: use ===

MatthewHerbst · 2019-01-10T23:00:32Z

src/client.js

-    try { return new ActiveXObject('Msxml2.XMLHTTP'); } catch(e) {}
+  if (
+    root.XMLHttpRequest &&
+    (!root.location || root.location.protocol != 'file:' || !root.ActiveXObject)


Nit: use !==

niftylettuce · 2019-01-11T00:48:56Z

Ill double check this before I publish. Thank you for your review. Super super helpful to have extra set of eyes.

…

On January 11, 2019 12:47:21 AM UTC, Matthew Herbst ***@***.***> wrote: MatthewHerbst commented on this pull request. > + }, + "prettier": { + "singleQuote": true, + "bracketSpacing": true, + "trailingComma": "none" + }, + "remarkConfig": { + "plugins": [ + "preset-github" + ] + }, + "repository": { + "type": "git", + "url": "https://github.com/visionmedia/superagent" + }, + "scripts": { It seems like what I'm looking for should be the default behavior: From [the `fixpack` docs](https://www.npmjs.com/package/fixpack): > It will re-write your package.json file as follows: > > - name first > - description second > - version third > - author fourth > - all other keys in alphabetical order > - dependencies and devDependencies sorted alphabetically > - newline at the end of the file It seems you could create a `.fixpackrc` file to manage the configuration. Personally I just manage `package.json` files manually (save for `npm install` runs) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1423 (comment)

fmal · 2019-03-25T12:02:27Z

@niftylettuce what's the status of this? will 5.0 be released soon?

niftylettuce · 2019-03-25T13:20:52Z

Will try to get this released today.

niftylettuce · 2019-03-28T07:40:11Z

v5.0.0 was just released to NPM and an updated README is now available here on GitHub (see https://github.com/visionmedia/superagent#superagent).

What's new

Browser version is now only 19KB (that's 60% in file size savings from v4.x which was 48KB)!
Codebase refactored using xo, prettier, remark, and more
Browser compatibility linting via eslint-plugin-compat
Browserslist integration (current targets are > 1%, last 2 versions, ie 9)

How to install

npm install superagent@latest

Or if you're using yarn:

yarn add superagent@latest

Polyfills

Please see the Required Browser Features section in the README if you plan to support IE 9 and/or IE 10 and use superagent.

niftylettuce · 2019-03-28T07:59:24Z

btw @MatthewHerbst I fixed all those weird syntax and equality issues from the old version, e.g. == vs ===

MatthewHerbst · 2019-03-28T18:09:22Z

@niftylettuce thanks so much for your hard work on this! Excited to upgrade at my company today 🚀

feat: refactored to support node/browser targets using babel

e73da09

niftylettuce mentioned this pull request Oct 1, 2018

thoughts for src folder and new setup with babel? (example link included) #1421

Closed

niftylettuce added a commit to lassjs/eslint-config-xo-lass that referenced this pull request Oct 1, 2018

fix: removed engines per error with CI builds

f66b21b

ladjs/superagent#1423

fix: fixed issue with "latest" versions specified in package.json

947293d

niftylettuce added 2 commits October 1, 2018 17:41

chore: fixed Readme.md -> README.md

4f7a99a

fix: removed code coverage threshold checks in nyc

0cc3aef

chore: switch from v6.4+ to v8.8.1+ due to http2 module

5cf9453

docs: fixed build comparison (48KB to 19KB)

bf8b9a2

kornelski closed this Jan 2, 2019

niftylettuce mentioned this pull request Jan 10, 2019

Precompiling node_modules troublesome, include browser ready version in npm? #1437

Closed

MatthewHerbst reviewed Jan 10, 2019

View reviewed changes

fmal mentioned this pull request Mar 25, 2019

Error in IE 11 when using latest @vue/cli to compile #1470

Closed

niftylettuce reopened this Mar 25, 2019

sbusch mentioned this pull request Mar 26, 2019

Fix @types/superagent has no default export mtennoe/swagger-typescript-codegen#64

Merged

niftylettuce closed this Mar 28, 2019

feat: refactored to support node/browser targets using babel #1423

feat: refactored to support node/browser targets using babel #1423

Conversation

niftylettuce commented Oct 1, 2018

niftylettuce commented Oct 1, 2018

niftylettuce commented Oct 1, 2018

niftylettuce commented Oct 1, 2018

niftylettuce commented Oct 1, 2018

niftylettuce commented Oct 1, 2018

codecov-io commented Oct 2, 2018 • edited Loading

Codecov Report

niftylettuce commented Oct 2, 2018

niftylettuce commented Oct 2, 2018

kornelski commented Oct 3, 2018

niftylettuce commented Oct 3, 2018

larron commented Dec 7, 2018

kornelski commented Jan 2, 2019

niftylettuce commented Jan 10, 2019

MatthewHerbst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftylettuce Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftylettuce commented Jan 11, 2019 via email

fmal commented Mar 25, 2019

niftylettuce commented Mar 25, 2019

niftylettuce commented Mar 28, 2019

What's new

How to install

Polyfills

niftylettuce commented Mar 28, 2019

MatthewHerbst commented Mar 28, 2019

codecov-io commented Oct 2, 2018 •

edited

Loading

niftylettuce Jan 11, 2019 •

edited

Loading