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

Feature/eslint config package #545

Merged
merged 132 commits into from Jan 29, 2024
Merged

Conversation

miguelmota
Copy link
Member

@miguelmota miguelmota commented Jan 10, 2024

  • adds shared .eslintrc.js and tsconfig.json to the monorepo root for the packages to extend and use. The only additional configuration is in frontend that also extends eslint react plugins and rules.
  • removes unused packages ipfs-worker/, and sdk-demo-api/ (in favor of api/ which is basically the same thing)
  • most of the modified files are due to eslint --fix changes
  • merges PR 541 into this one

@shanefontaine
Copy link
Member

  • Remove custom type definitions (our own types dirs). Simplifies tsconfig.json by removing the need to define them but also needing to redefine the defaults, like node_modules/types. Most updated packages have correct types. Added // @ts-expect-error to a few imports that did need it. In theory, IMO if something doesn't have types we should be hesitant to use it as that means it is unmaintained and can cause headaches elsewhere.

  • In public packages (core, sdk), use the files option in package.json as an NPM allowlist (published files must be included in files and not excluded in .npmignore).

  • Use rm -rf for NPM scripts instead of rimraf. Removes possible debugging steps at the cost of compatibility.

    • I recognize that this breaks compatibility with non-Unix machines. However, (1) my understanding is that WSL is used by a decent amount of people on Window's these days (even more-so in OS world) and (2) I believe many other parts of the packages would not work with non-Unix, such as paths and env vars. I think npx rimraf makes sense for a lifecycle script, but can be reintroduced if/when we non-Unix-ify the repo.
    • I hit a case where rimraf wasn't working because I had stopped an install halfway through. node_modules knew about rimraf but didn't have all of its deps. In this case, npx used my node_modules rimraf instead of global, but it was corrupted. npx wouldn't ever try global because it still "saw" it locally. Error message that was difficult to understand.

@shanefontaine
Copy link
Member

@miguelmota final commits have been pushed:

  • Updated to pnpm
  • Simplify/standardize scripts and deps + general cleanup

@shanefontaine
Copy link
Member

shanefontaine commented Jan 28, 2024

@miguelmota final round of changes. Ready to merge at any time now.

  • Remove binary from hop-node. Replaced with ts-node for local build/test and node for prod and docker.

    • IIUC binaries let someone run hop-node without a node env and w/o docker. I am under the impression that nobody does this (at this time). Current options to run hop-node is either:
      • Pull repo, build, run
      • Pull docker, run (build was already completed by CI prior to pushing to dockerhub)
    • IIUC the current bin calls into the build dir but instead should be using a fully compiled bin instead. If reintroduced, consider using nexe or some package that builds an executable bin.
    • Fun fact: I believe the latest version of node has experimental binary support built-in (though likely won’t be stable for many years)
  • Use swc for hop-node compile instead of Native Typescript for a 3.7x increase (5.39s → 1.46s) in hop-node build time (useful during dev and testing).

    • Generally goes against the goal of using natives/defaults, but feels practical in this case:
      • Meaningful speed increase (3.7x vs native transpileOnly and 6.7x vs how we were doing it)
      • Suggested as a main option by ts-node when using ts-node
      • No config (only an NPM package to download)
  • RM hop-node and stats-worker package_check.sh since I believe they are now unused

  • (Can be reverted) (SDK only) Use esbuild instead of babel for bundling + CDN

    • Gets rid of 65% of devDeps in SDK (40% of total deps) for faster / safer / easier installs & builds.
  • Various package updates to non-outdated versions

@shanefontaine shanefontaine mentioned this pull request Jan 28, 2024
@miguelmota
Copy link
Member Author

miguelmota commented Jan 29, 2024

Remove binary from hop-node. Replaced with ts-node for local build/test and node for prod and docker.

That seems fine. I think the Dockerfiles need to be updated to reflect that change.

IIUC the current bin calls into the build dir but instead should be using a fully compiled bin instead. If reintroduced, consider using nexe or some package that builds an executable bin.

I think that the bin/ folder in node apps are a convention to denote the entrypoint for the cli.
For example, the lodestar client has bin/lodestar.js and in package.json they have "bin": {"loadestar": "bin/loadstar.js"}} for global command. In their Dockerfile they have the bin path as the entrypoint; ENTRYPOINT ["node", "./packages/cli/bin/lodestar"].
In linux, the /bin folder means it contains executables and doesn't necessarily have to be compiled binary code. For example, /bin/pip is a readable python interpreted program (starts with #!/usr/bin/python) and it's not complied binary.
Just wanted to point that out to explain the reason it was like that.

Use swc for hop-node compile instead of Native Typescript for a 3.7x increase (5.39s → 1.46s) in hop-node build time (useful during dev and testing).

Is swc only used during start script and not pnpm build? Seeing tsc being used instead of swc on build.

DeepinScreenshot_select-area_20240129013421

(Can be reverted) (SDK only) Use esbuild instead of babel for bundling + CDN

Seems good to me 👍

I see that sdk CDN docs from README were removed. Any reason? For example, ethers has cdn links/examples in docs

@@ -54,7 +52,9 @@ import './withdrawalProof'
import './metrics/bonderBalance'
import './metrics/bonderTxCost'

program.version(packageJson.version)
// TODO: Since package.json version isn't updated, it shouldn't be used here
const version = '0.0.1'
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe the git revision can be used as version here

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Updated with 2ac8579.

@shanefontaine
Copy link
Member

shanefontaine commented Jan 29, 2024

I think the Dockerfiles need to be updated to reflect that change.

Updated with 9768d45. I believe that works, but plan on doing a quick pass on the Docker stuff today, most just for my understanding.

bin

That is interesting and I did not know that. I thought bin 100% implied only binary files but see now I was incorrect. Thanks for the info 🙏

Is swc only used during start script and not pnpm build? Seeing tsc being used instead of swc on build.

Good catch, I didn't realize this. I just went to add it but then realized that it (understandably) completely ignores tsconfig and uses its own config that can be modified. I'm not too stoked on the mental overhead of "every package in Hop follows tsconfig except hop-node". But now also don't love that our local build/test is different than prod build 🤔 . Let me think about this but if you have thoughts I'm down.

One note is that there is a native way to do this too. Our develop branch builds are ~9.5s and our swc builds are 1.46s. However, our develop branch with ts-node --transpileOnly (i.e. don't look for type errs during build) takes 5.39s. Don't necessarily think that is a better solution, but think it is interesting.

I see that sdk CDN docs from README were removed

I had removed bundling completely for a bit and removed from the README at that time. I dove in a bit and now have a better understanding so I added the bundle back but forgot to reintroduce to the README. Reintroduced with 2ac8579.

@miguelmota
Copy link
Member Author

swc

Maybe there's a way to autogenerate the swc config from tsconfig, such as creating a small script to map tsconfig options to swc config options and output to .swcrc. I like the speed improvements and wouldn't be opposed for hop-node to use a different build system.

@shanefontaine shanefontaine merged commit 117963e into develop Jan 29, 2024
0 of 8 checks passed
@shanefontaine shanefontaine deleted the feature/eslint-config-package branch January 29, 2024 20:24
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