Skip to content

refactor: remove axios - #1237

Merged
benjlevesque merged 13 commits into
masterfrom
refactor/remove-axios
Jan 18, 2024
Merged

refactor: remove axios#1237
benjlevesque merged 13 commits into
masterfrom
refactor/remove-axios

Conversation

@benjlevesque

@benjlevesquebenjlevesque commented Nov 13, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

  • reduce size (minor impact)
  • avoid versions conflicts
  • reduce maintenance

Changing to MSW for mocks also has the benefit to be compatible with any client.

Choices

  • Dropping gas-price-definer. This is never used, since feat!: drop legacy storage #1117
  • dropping IpfsStore.read. This is never used.
  • remove maxSize from IpfsManager.read. This method is kept as a convenience, as it's useful to test the read after upload, but is no longer used (since feat!: drop legacy storage #1117).
  • axios remains a devDependency of smart-contracts and toolbox for now

@benjlevesquebenjlevesque changed the title refactor/remove axiosbuild(deps): Bump axios from 0.27.2 to 1.6.0 refactor: remove axiosNov 13, 2023
@benjlevesquebenjlevesque changed the title build(deps): Bump axios from 0.27.2 to 1.6.0 refactor: remove axiosrefactor: remove axiosNov 13, 2023
benjlevesque
benjlevesque commented Nov 13, 2023
View reviewed changes
Comment threadpackages/request-node/src/request/getChannelsByTopic.ts Outdated
benjlevesque
benjlevesque commented Nov 13, 2023
View reviewed changes
Comment threadpackages/request-node/src/request/persistTransaction.ts Outdated
benjlevesque
benjlevesque commented Nov 13, 2023
View reviewed changes
Comment threadpackages/request-node/test/persistTransaction.test.ts
.expect(StatusCodes.INTERNAL_SERVER_ERROR);
});

it('should catch IPFS timeout error', async () => {

@benjlevesquebenjlevesqueNov 13, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I decided to remove this test. it's a bit hard to replicate and I feel it has no real value

@MantisCloneMantisCloneJan 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I disagree with this. The Request Node still writes to IPFS but it should fail gracefully if for some reason that write fails.

Comment threadpackages/ethereum-storage/test/ipfs-manager.test.ts Outdated
benjlevesque
benjlevesque commented Nov 16, 2023
View reviewed changes
Comment threadpackages/request-client.js/test/index.test.ts
Comment on lines -187 to -190
expect(config.headers[httpConfigDefaults.requestClientVersionHeader]).toBe(
packageJson.version,
);

@benjlevesquebenjlevesqueNov 16, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I ditched that test, I don't really see the intention

benjlevesque
benjlevesque commented Nov 16, 2023
View reviewed changes
Comment threadpackages/request-client.js/test/index.test.ts Outdated
@MantisCloneMantisClone mentioned this pull request Nov 16, 2023
Closed
@benjlevesque
benjlevesque changed the base branch from master to test/reduce-flaky-testsNovember 28, 2023 16:17
Base automatically changed from test/reduce-flaky-tests to masterDecember 19, 2023 11:29
@coveralls

coveralls commented Jan 17, 2024•
edited
Loading

Copy link
Copy Markdown

Coverage Status

coverage: 86.667% (-0.5%) from 87.128%
when pulling 8e03773 on refactor/remove-axios
into 45d1f5b on master.

@benjlevesque
benjlevesque marked this pull request as ready for review January 17, 2024 15:40
yomarion
yomarion approved these changes Jan 17, 2024
View reviewed changes

@yomarionyomarion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice

@yomarion

yomarion commented Jan 17, 2024

Copy link
Copy Markdown
Contributor

FYI @benjlevesque, the yarn.lock file still references axios@0.27.2 that looks unused.

@benjlevesque

benjlevesque commented Jan 17, 2024

Copy link
Copy Markdown
ContributorAuthor

@yomarion

  • axios remains a devDependency of smart-contracts and toolbox for now

alexandre-abrioux
alexandre-abrioux approved these changes Jan 17, 2024
View reviewed changes

@alexandre-abriouxalexandre-abrioux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice! ✨

Comment threadpackages/request-client.js/test/index.test.ts Outdated
mockServer.close();
});

it('specify the Request Client version in the header', async () => {

@alexandre-abriouxalexandre-abriouxJan 17, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I couldn't find where we assert in this test that the client version is passed down in the headers. Is the test name wrong, or is this assertion missing?

@MantisCloneMantisCloneJan 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What was the conclusion here?

@alexandre-abriouxalexandre-abriouxJan 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it was a typo and it was renamed back to the original name. cc @benjlevesque can you confirm?

@benjlevesquebenjlevesqueJan 22, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yes, sorry my comment remained "pending":

good catch. Probably a bad merge or copy paste, I removed that test

but in the meantime I also added back a check on the version header

Comment threadpackages/request-client.js/test/index.test.ts Outdated
@benjlevesque
benjlevesque enabled auto-merge (squash) January 18, 2024 10:12
@benjlevesque
benjlevesque merged commit 2e2bcfa into masterJan 18, 2024
@benjlevesque
benjlevesque deleted the refactor/remove-axios branch January 18, 2024 10:18
@benjlevesquebenjlevesque mentioned this pull request Jan 19, 2024
Merged
@MantisCloneMantisClone mentioned this pull request Jan 30, 2024
Merged
@MantisCloneMantisClone mentioned this pull request Jul 17, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yomarionyomarionyomarion approved these changes
@KolevDarkoKolevDarkoAwaiting requested review from KolevDarko
@MantisCloneMantisCloneAwaiting requested review from MantisClone
@leoslrleoslrAwaiting requested review from leoslr
@kevindaveekevindaveeAwaiting requested review from kevindavee
@olivier7delfolivier7delfAwaiting requested review from olivier7delf
+1 more reviewer
@alexandre-abriouxalexandre-abriouxalexandre-abrioux approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benjlevesque@coveralls@yomarion@MantisClone@alexandre-abrioux