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

Origin/feat/refactoring #108

Merged
merged 12 commits into from
Sep 26, 2023

Conversation

0xkf
Copy link
Contributor

@0xkf 0xkf commented Sep 23, 2023

combined 2 directories (node and browser) to 1 directory(browser_node)
When you hit "npm run node" , it will work on node environment.
When you hit "npm run browser " , it will work on browser environment.

I edited that setting type module and compiling typescript code to javascript for node environment.

I would like to hear your feedback.
Thank you for reading

add ethers, hardhat,
type module,
then edited npm run command
compiled typescript to javascript.
This is to make it easy to call
node function.
deleted node directory, changed the name of refactored directory.
Copy link
Member

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Hey @0xkf, thanks for your excellent work! I think it can be merged with one more thing: can you remove examples/browser and examples/node? Also, can you put everything under examples/browser_node/ to examples/ and remove example/browser_node/?

@mhchia mhchia linked an issue Sep 25, 2023 that may be closed by this pull request
@0xkf
Copy link
Contributor Author

0xkf commented Sep 26, 2023

Hey @0xkf, thanks for your excellent work! I think it can be merged with one more thing: can you remove examples/browser and examples/node? Also, can you put everything under examples/browser_node/ to examples/ and remove example/browser_node/?

Thank you for your review!
OK!
I will do that soon!

form "npm run test"
to "npm run node" or "npm run browser"
changed  from "example/node_browser " directory
to "example" directory
(take note that you should hit "npm run clean" after hitting "npm run browser")
@0xkf
Copy link
Contributor Author

0xkf commented Sep 26, 2023

I changed as you said.
Can you review??
@mhchia

Copy link
Member

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Directory changes look good! It came to my mind that READMEs should also be updated. Could you do them altogether in this PR?

For examples/README.md, I've made a draft for it and you can use it directly.

For README.md, the Example section can be changed from

See [example for both NodeJS](./examples/node/) and [example for browser](./examples/browser/).

to

See the [example](./examples) that can be run in both NodeJS and browser.

Let me know too if you have an idea of what we should write for them

3. Run the example in a browser
4. Run the example in NodeJS
...
@0xkf
Copy link
Contributor Author

0xkf commented Sep 26, 2023

I have updated README, watching your draft.
Could you review??

@0xkf
Copy link
Contributor Author

0xkf commented Sep 26, 2023

@mhchia

Copy link
Member

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Sorry, I missed that we should also remove *.js like examples/src/configs.js and examples/src/index.js. I've also added one comment about your note in the readme. Should be ready after these

@@ -20,19 +20,18 @@ In a new terminal, run:
$ npx hardhat node
```

3. Run the web server
3. Run the example in a browser
(take note that you should hit "npm run clean" after hitting "npm run browser")
Copy link
Member

Choose a reason for hiding this comment

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

Is this note still relevant? If so, I'm thinking we can rephrase it to 'Try npm run clean if you've made changes to the code but the web page is not updated'

delete type module
add  npm run node command
""tsc --module commonjs && node dist/src/index.js""
@0xkf
Copy link
Contributor Author

0xkf commented Sep 26, 2023

I have updated as you said.
・deleted js files and realized npm run node experience without that js files.
・updated Readme as you said
@mhchia

@mhchia mhchia merged commit 9b6f8ea into Rate-Limiting-Nullifier:main Sep 26, 2023
2 of 3 checks passed
@mhchia
Copy link
Member

mhchia commented Sep 26, 2023

Merged. Thanks for your contribution

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.

Refactor examples
2 participants