-
Notifications
You must be signed in to change notification settings - Fork 94
Conversation
@volovyk-s @vgrichina |
@willemneal I can see that you are improving this PR constantly. Please, ping me when it will be ready for review. |
@volovyk-s Was just about to! It should be ready to go. I just can't trigger CI. |
commands/repl.js
Outdated
console.error(error); | ||
process.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error(error); | |
process.exit(1); | |
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that throwing the error would cause all documentation to print.
commands/repl.js
Outdated
console.error(error); | ||
process.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error(error); | |
process.exit(1); | |
throw error; |
@willemneal run |
Co-authored-by: Serhii Volovyk <SergeyVolovyk@gmail.com>
@volovyk-s. If I throw the error I get
vs just the last bit. If this is the expected output then I'll definitely switch, but it seems pretty noisy to me. |
Yes, it's noisy, but I would rather be consistent and optimize it at some point. |
@volovyk-s Done! |
@volovyk-s Any other blockers? |
@willemneal reviewed and tested, really cool feature. The only thing that I would add is an example or description of how users expected to use it. For example, I have built this simple script: import { Context } from "../context";
export async function main({ account, near, nearAPI, argv }: Context) {
const state = await account.state();
console.log(state.storage_usage);
} And it workes fine, but only because I created this script inside of the |
Ah great point! I wonder if you can access the type globally. I'll give it a try and otherwise will add the documentation. My only fear is that the global install could differ from the local one, but I don't think these types will change that much. That is unless we change NAJ significantly. |
@volovyk-s Couldn't figure it out and perhaps if the version mismatch is an issue we can have the global installation check the local version and complain. So for now I added info to the README.md |
Got it, I'm wondering if there is a way to write such scripts as a single .js file. We can drop TS support, imports, and main function if needed. |
The ts is optional. In fact the types import is optional since it could be typed any or typed with naj. With js no imports are required since naj is passed. See the test js file. Regardless this is still a leg up on Naj since you have your local keystore, can use a ledger, and get an accpunt already set up. Scripts with just naj and no cli are hacky and fragile. |
Would it make sense to move the Context type to Naj? |
Oh misread your comment when I first read it. I think that main function is needed and what a script will end up using since top level async support is not standard. |
@volovyk-s Thoughts? |
@volovyk-s ^ |
Sorry for the delay, @willemneal . CLI is on hold for now. Good point ^. I will merge it, maybe will add some tweaks later. Thank you once more for your contribution, this is an important feature. |
Many users end up creating their own scripts which hack together the information provided from near-cli. Currently accepts javascript or typescript. The script file just needs to export a
main
function.This is particularly useful for using a ledger in a script.
You can also pass arguments after
--
E.g.