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

feat: add script to update all polkadot-js deps, and resolutions to latest #63

Merged
merged 13 commits into from
Apr 21, 2022

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Apr 14, 2022

Add a script that will recursively lazy search for all package.json files in a directory and update all polkadot-js deps, and resolutions to their latest versions.

Args

--path -> path to directory, defaults to ./

Note: This has been tested against txwrapper-core and substrate-api-sidecar, and works correctly.

This script by deafult will ignore the following directories:

node_modules
build
lib

yield;
} else if (path.split('/').slice(-1)[0].startsWith('.')) {
// Do no traverse any hidden directories
yield;
Copy link

Choose a reason for hiding this comment

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

Perhaps it’s best to ‘continue’ than have this (and the above) empty yield, so that we don’t get some undefined’s coming back from this generator, just paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, you are totally correct. I had a brain fart when writing this and totally forgot about continue and handled the undefined instead. Fixed that, also removed the check for the node_modules file, and just added a ignorePaths param. Therefore we can account for main paths such as generated build, or lib folders.

* @returns
*/
function fetchReleaseTag(link) {
const cmd = `curl --silent ${link}`;
Copy link

Choose a reason for hiding this comment

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

Not a big deal, but can we avoid depending on curl and use the http package?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea this totally makes sense. My first iteration through this, I actually used the http package, but then changed it to curl since it cleaned it up a little bit. Doing some basic digging too, it doesn't make sense to call curl from a node process. Looked at some benchmarks done by other users, and its quite faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay cool, I added a native https request, and wrapped it in a promise. Lmk what you think. I like it a lot better thats for sure.

}

/**
* Generator function to recursively lazy iterate through each directory. This searches
Copy link
Member

Choose a reason for hiding this comment

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

It this recursive variant an idiom in JS?

It's elegant and nice but I would still prefer iterative variant...

Copy link
Member Author

Choose a reason for hiding this comment

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

It this recursive variant an idiom in JS?

I would say in some senses yes. When it comes to most kinds of traversing, recursion is pretty popular. The main focus being, it's a little easier to maintain and read IMO. Where it could be an issue is where memory intensive processing is happening and can cause memory leaks. In this case though we are void of that.

It's elegant and nice but I would still prefer iterative variant...

In most cases I would prefer the iterative variant as well, but in this case for readability and maintenance I would say lets keep it recursive. It does a nice job of establishing the lazy iterator without any confusing logic.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, this is nice Tarik to avoid to do this manually :)

@TarikGul TarikGul merged commit f26cd26 into main Apr 21, 2022
@TarikGul TarikGul deleted the tarik-update-deps-script branch April 21, 2022 00:54
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.

4 participants