cleanup: clean up a bit of the mess i created #37
Conversation
| @@ -42,22 +39,16 @@ | |||
| "tsup": "^8.3.5", | |||
| "typescript": "^5.6.3", | |||
| "viem": "^2.21.48", | |||
| "vitest": "^2.1.4" | |||
| "vitest": "^2.1.4", | |||
| "dotenv": "^16.4.5" | |||
There was a problem hiding this comment.
why is everything devDependencies?
When using as a lib, i would expect viem to be a peerDependency.
There was a problem hiding this comment.
Because of how tsup works. DevDependencies are bundled up automatically and as we're only using the deps for some constants, IMO that is much nicer for downstream users.
There was a problem hiding this comment.
This is particularly useful for actions-usage, where no dependency resolution takes place.
There was a problem hiding this comment.
i see, but currently, this means then when using as js lib it would contain the inlined viem + the viem the package uses?
It#s not a big problem as currently we only use this is node modules were pkg size does not matter, but perhaps that's sth we should look into down the line.
There was a problem hiding this comment.
Depending on if tree shake works, I'd say.. does the whole view land in there or the constants we need?
package.json
Outdated
| "src/lib.ts", | ||
| "src/cli.ts" | ||
| ], | ||
| "entry": ["src/action.ts", "src/lib.ts", "src/cli.ts"], | ||
| "splitting": false, | ||
| "sourcemap": false, | ||
| "clean": true, | ||
| "dts": true, | ||
| "treeshake": true, |
There was a problem hiding this comment.
not sure if that helps with anything
There was a problem hiding this comment.
If it really doesn't do anything, that should be easily testable. Does removing it change the bundle (size)?
package.json
Outdated
| "src/lib.ts", | ||
| "src/cli.ts" | ||
| ], | ||
| "entry": ["src/action.ts", "src/lib.ts", "src/cli.ts"], | ||
| "splitting": false, | ||
| "sourcemap": false, |
There was a problem hiding this comment.
why no sourcemaps?
There was a problem hiding this comment.
No particular reason, but mostly expected ts-users who likely will consume d.ts anyway.
| @@ -1,4 +1,5 @@ | |||
| #!/usr/bin/env node | |||
| import "dotenv/config"; | |||
There was a problem hiding this comment.
not quite sure if it should be here ... probably not and one should export ALCHEMY_API_KEY before running the cli or put the api key as parameter on the script. Didn't want to add commander or similar for such a simple task though and working with pure args in node is pain :/
I'm conflicted.
| @@ -102,4 +102,4 @@ export const getRPCUrl = ( | |||
| } catch (e) {} | |||
| }; | |||
|
|
|||
| export { ChainId, ChainList }; | |||
| export { ChainId, ChainList, type SupportedChainIds }; | |||
There was a problem hiding this comment.
noticed it would be useful to have this type.
@boredland sorry we had some issues with this action today, so i jumped the gun and merged without reviews to make things work again. If you got some time, please check the past prs and let me know what you don't like 😅
When committing I currently have to do it via --no-verify as otherwise the precommit errors with
I indeed don't want to add them.
Ofc i could turn the message off, but i did not understand where it comes from - perhaps you got an idea.