rocicorp / replicache

Realtime Sync for Any Backend Stack
https://doc.replicache.dev
1.02k stars 37 forks source link

wat #969

Closed phritz closed 2 years ago

phritz commented 2 years ago

This is a typescript thing I do not understand. The cli.ts file invokes licensingCLI.main() without the required argument. In VSCode that line is red showing me the expected error Expected 1 arguments, but got 0. An argument for 'licenseServerURL' was not provided.. However it compiles just fine and then fails at runtime when the missing argument is attempted to be used. To see this:

  1. rm $(which replicache)
  2. rm out/cli.cjs
  3. npm run build
  4. npm link
  5. npx replicache get-license
  6. enter data at the prompts
  7. notice that in the final step when it goes to use the missing argument it fails:
    Fetching License Key...
    Sorry, there was an error. Please file a bug at https://github.com/rocicorp/replicache/issues/new with the following information:
    TypeError [ERR_INVALID_URL]: Invalid URL

    The "invalid url" is undefined.

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
replicache ✅ Ready (Inspect) Visit Preview Apr 18, 2022 at 11:38PM (UTC)
phritz commented 2 years ago

Interesting that CI catches the problem (this time, it didn't a few weeks ago?!) bc it runs check-types. But shouldn't build check the types?!

aboodman commented 2 years ago

I think this is just a code factoring / terminology thing.

The build step doesn't include type checking. That might seem crazy, but in js land everything is configurable, swappable, customizable, etc. And @arv has opted to use esbuild for compilation for ... reasons ... instead of tsc (I think partly because it's super fast, but probably other reasons too), but esbuild doesn't do type checking. So we use tsc to do that. But the build script doesn't seem to include that step:

https://github.com/rocicorp/replicache/blob/main/tool/build.mjs

aboodman commented 2 years ago

(I agree that intuitively, I would expect npm run build to do type checking. Not sure if there's a reason it doesn't currently, or just accidental).

phritz commented 2 years ago

Seems weird to go out of our way to use a compile-time type language and then not use it at compile time!

phritz commented 2 years ago

@arv anything more that I should understand here?

arv commented 2 years ago

I think we should change our build script to also run the type checking.

phritz commented 2 years ago

I think we should change our build script to also run the type checking.

do you mean just having a rule like:

"prebuild": "npm run check-types"

or do you mean monkeying with the esbuild parameters in https://github.com/rocicorp/replicache/blob/main/tool/build.mjs?

arv commented 2 years ago

prebuild works for me but there might be some reconciliation that should be done. I can take over if you want?

phritz commented 2 years ago

prebuild works for me but there might be some reconciliation that should be done. I can take over if you want?

yes please and thanks :)