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

Make arweave-js more robust against axios default changes (such as from erdjs) #97

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

pctj101
Copy link
Contributor

@pctj101 pctj101 commented Aug 2, 2021

Main change is in
src/common/transactions.ts

But I had problems getting npm 7 to compile, so I had to change the unmaintained BabelMinify to TerserPlugin

npm test passes with all 46 tests suceeding


The challenge is using arweave along with elrond's erdjs. While there is nothing "wrong" with arweave-js, there is a simple tweak that can be done to prevent errors.

erdjs introduces a changes in 2 files to axios which causes arweave-js to fail

https://github.com/ElrondNetwork/elrond-sdk-erdjs/blob/main/src/apiProvider.ts#L94
https://github.com/ElrondNetwork/elrond-sdk-erdjs/blob/main/src/proxyProvider.ts#L184

// See: https://github.com/axios/axios/issues/983
axios.defaults.transformResponse = [
  function(data) {
    return JSONbig.parse(data);
  },
];

This leads to trouble when arweave-js tries to get a string from https://arweave.net/tx_anchor which results in a string which can't parse into JSON.

Example Response:

8TIgL9MZbyLsVQG240q66i8bu9deW51OLp0_ndUUS2hVvorTJBDcspiFU4BfQ0lp

Example Error:

{name: "SyntaxError", message: "Syntax error", at: 2, text: "8TIgL9MZbyLsVQG240q66i8bu9deW51OLp0_ndUUS2hVvorTJBDcspiFU4BfQ0lp"}
at: ​2
message: ​"Syntax error"
name: ​"SyntaxError"
text: ​"8TIgL9MZbyLsVQG240q66i8bu9deW51OLp0_ndUUS2hVvorTJBDcspiFU4BfQ0lp"

Pulling out the axios main packge into window.axx, then forcing the defaults.transformResponse to []  fixes the issue

axx.defaults.transformResponse = []
arwewave.api.request().get("tx_anchor").then((e) => console.log(e)) -> prints out tx_anchor without issue

Since we know that arweave-js will interoperate with many more other libaries, we can make arweave-js more robust by ensuring transformResponse = []  where we know it is not necessary.  As such, we can start with get("tx_anchor") which we know has an issue.

@hlolli
Copy link
Contributor

hlolli commented Oct 9, 2021

looks good, can you rebase on master so I can see if the tests pass?

@pctj101
Copy link
Contributor Author

pctj101 commented Oct 9, 2021

Let me see if I can remember what I did and get that to you. Thanks for writing back!

@pctj101
Copy link
Contributor Author

pctj101 commented Oct 9, 2021

Rebase is in :)

@hlolli
Copy link
Contributor

hlolli commented Oct 10, 2021

haha thanks, I realized as I was reviewing another PR, that I forgot to add a key to actually run the test on pull requests. I have no doubt the test will succeed, but it's better to merge with the lights turned on green, so once more, could you rebase again to master 🙏

@pctj101
Copy link
Contributor Author

pctj101 commented Oct 10, 2021

Rebased!

@hlolli hlolli merged commit 9b0e3cb into ArweaveTeam:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants