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

chore(dependencies): Update superagent version #114

Merged
merged 5 commits into from
Aug 1, 2023
Merged

Conversation

Tetsu9901
Copy link
Contributor

We want to update the superagent version

@Tetsu9901 Tetsu9901 added the wip label Jul 28, 2023
@Tetsu9901 Tetsu9901 self-assigned this Jul 28, 2023
Comment on lines 96 to 112
} else {
// superagent < 3.6

if (isNodeServer) { // node server
const originalPath = this.path;
this.path = this.url;
this._appendQueryString(this); // use superagent implementation of adding the query
path = this.path; // save the url together with the query
this.path = originalPath; // reverse the addition of query to path by _appendQueryString
} else { // client
const originalUrl = this.url;
this._appendQueryString(this); // use superagent implementation of adding the query
path = this.url; // save the url together with the query
this.url = originalUrl; // reverse the addition of query to url by _appendQueryString
}
Copy link
Contributor Author

@Tetsu9901 Tetsu9901 Jul 28, 2023

Choose a reason for hiding this comment

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

I change only this in this file

All other modification is prettier

@Tetsu9901 Tetsu9901 removed the wip label Jul 28, 2023
@Tetsu9901 Tetsu9901 marked this pull request as ready for review July 28, 2023 13:54
@Tetsu9901 Tetsu9901 added the dependencies Pull requests that update a dependency file label Jul 28, 2023
@Tetsu9901 Tetsu9901 force-pushed the update-superagent branch 2 times, most recently from 615432a to d0ee3c8 Compare July 31, 2023 09:20
@Tetsu9901 Tetsu9901 requested a review from fdubost July 31, 2023 09:23
.eslintrc Outdated
@@ -7,7 +7,6 @@
"no-underscore-dangle": 0,
"no-use-before-define": [2, "nofunc"],
"max-len": [2, 150, 2],
"quotes": [2, "single"],
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? 👀 It generates a lot of noise in this PR 😕

Copy link
Contributor Author

@Tetsu9901 Tetsu9901 Jul 31, 2023

Choose a reason for hiding this comment

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

My prettier modifie all files with " " instead of ' ' and
I can't do this :

"quotes": [2, "single", "double"]

So I removed it instead of revert all prettier modifications

Copy link
Member

Choose a reason for hiding this comment

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

I just saw your comment above but it is not a good reason to change lint rules in this project 😄 You should find why your prettier is misconfigured. Maybe a .prettierrc is missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was planned but you are too fast for review 😅

@@ -10,8 +10,8 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [10.x, 12.x, 14.x, 15.x]
Copy link
Member

Choose a reason for hiding this comment

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

We should maybe keep the compatibility with node 16 because it's a LTS 🦒

package.json Outdated
@@ -16,10 +16,10 @@
"url": "https://github.com/M6Web/superagent-mock"
},
"engines": {
"node": ">=10"
"node": "^18.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

As said above, maybe 16 is better

@Tetsu9901 Tetsu9901 added the wip label Jul 31, 2023
@Tetsu9901 Tetsu9901 force-pushed the update-superagent branch 4 times, most recently from 5b2362d to 008667d Compare July 31, 2023 09:55
.prettierrc Outdated
Comment on lines 1 to 6
{
"singleQuote": true,
"printWidth": 120,
"trailingComma": "all",
"bracketSpacing": false
}
Copy link
Contributor Author

@Tetsu9901 Tetsu9901 Jul 31, 2023

Choose a reason for hiding this comment

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

This rules come from app-bedrock-web

Choose a reason for hiding this comment

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

It is a best practice to separate the refacto of prettier and the new code you want to add, here we can't see if a modification comes from the prettier refacto or not

},
"peerDependencies": {
"superagent": ">=3.6.0"
"superagent": ">=8.0.9"

Choose a reason for hiding this comment

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

Why is superagent a peer dependency instead of direct dependency ?

Copy link
Member

Choose a reason for hiding this comment

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

superagent-mock is not in responsibility for installing superagent in the project where it is used

.prettierrc Outdated
Comment on lines 1 to 6
{
"singleQuote": true,
"printWidth": 120,
"trailingComma": "all",
"bracketSpacing": false
}

Choose a reason for hiding this comment

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

It is a best practice to separate the refacto of prettier and the new code you want to add, here we can't see if a modification comes from the prettier refacto or not

yarn.lock Outdated

Choose a reason for hiding this comment

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

Are we not supposed to move all projects to pnpm ?

@@ -10,7 +10,6 @@ module.exports = function (request, config, isServer) {
currentLog = log;
};
var superagentPackage = require('superagent/package.json');
var superagentUserAgentHeader = isServer ? {'User-Agent': 'node-superagent/' + superagentPackage.version} : {};

Choose a reason for hiding this comment

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

Why does this have been removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user agent is no longer set by the superagent library, so there's no point in testing its presence on the superagent-mock side

@Tetsu9901 Tetsu9901 changed the title Update superagent version chore(superagent): Update superagent version Aug 1, 2023
@Tetsu9901 Tetsu9901 changed the title chore(superagent): Update superagent version chore(dependencies): Update superagent version Aug 1, 2023
@Tetsu9901 Tetsu9901 merged commit 19d6415 into master Aug 1, 2023
2 checks passed
@Tetsu9901 Tetsu9901 deleted the update-superagent branch August 1, 2023 08:28
@Tetsu9901 Tetsu9901 restored the update-superagent branch August 1, 2023 14:18
@Tetsu9901 Tetsu9901 deleted the update-superagent branch August 1, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community review dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants