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

modernize tooling #50

Closed
wants to merge 14 commits into from
Closed

modernize tooling #50

wants to merge 14 commits into from

Conversation

PuruVJ
Copy link
Collaborator

@PuruVJ PuruVJ commented Jul 1, 2024

Closes #49.

Builds on top of Phoebe's PR. We can merge it into the PR, or into main once Protobuffectomy's merged into main

  • Remove npm and yarn, replace with pnpm
  • type: module
  • jest -> vitest
  • Build files with tsup instead of tsc
  • Remove git husky, lint in GH actions
  • exports map in package.json
  • publint-compliant
  • Ships a /min folder which has minified bundle applied in production builds
  • Minimum node version set to v18(Previous LTS)
  • Removed all dependencies for the end-user.

Advantages

Reduced files

Before we were shipping entire project structure as is. Now we're shipping just 5 files:

  • index.js
  • index.d.ts
  • index.js.map
  • min/index.js
  • min/index.js.map

More deterministic

All the dependencies are now moved to devDependencies, which means end-users will not be installing those deps, they're bundled in the index.js file itself.

Required for webpack, and has the benefit of more deterministic end-user installations(If we specify brotli ^1.3 and are using 1.3.3, then only that very specific version gets shipped, whereas before, a user could have been gotten 1.3.4 which could have had a regression and cause errors)

Smaller

Our bundle size as reported was 109KB min+brotli. Now that we control the minification and tree-shaking, it's down to just 85KB min+br. In dev mode, user's bundler will use our unminified build. When building for prod, it will use the existing minified file, which can't get much smaller, so bundler-inconsistencies while minifying are also eliminated. All this is Vite and Parcel specific off-course, Webpack isn't advanced enough for this behavior yet, it will always get the unminified build and try to minify that.

It also uses standard import/export syntax.

What I didn't do:

  • Move the tests app and examples app to Vite(There's a PR open for that)
  • Integrate changesets for automatic releases and changelog. Follow-up PR for that
  • Monorepo-ize the codebase, which means tests run against how the package is published rather than a local version which may not always be accurate. Follow-up PR for that

@PuruVJ PuruVJ marked this pull request as draft July 1, 2024 11:43
@PuruVJ PuruVJ requested review from drogus and cloutiertyler July 1, 2024 11:54
@PuruVJ PuruVJ marked this pull request as ready for review July 1, 2024 11:58
Copy link
Collaborator

@drogus drogus left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Jul 1, 2024

Thanks, I'll still wait for Tyler's review before merging

EDIT: hang on, there's some weird issue with webpack, with Vite it works. Converting to draft to figure it out

EDIT 2: Webpack is so stupid

Edit 3: Figure it out, now we bundle all our deps into the index.js. Solves it for webpack as well

@PuruVJ PuruVJ marked this pull request as draft July 1, 2024 12:20
@PuruVJ PuruVJ marked this pull request as ready for review July 1, 2024 13:14
@PuruVJ PuruVJ requested a review from gefjon July 1, 2024 13:15
@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Jul 1, 2024

cc @gefjon could you test some of the stuff manually? I think you know it the best

@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Jul 1, 2024

Putting it back to draft as putting it on platform: browser just cut the bundle size by 50%!! Testing to see whether it works in nodejs as well

@PuruVJ PuruVJ marked this pull request as draft July 1, 2024 14:19
PuruVJ and others added 3 commits July 2, 2024 15:08
- Integrate protobufectomy, which replaces our protobuf and json WS message schemas
  with a SATS-ful schema for both BSATN and JSON.
- Notice that this SDK's implementation of JSON had diverged significantly
  from SpacetimeDB's use of `serde_json` leading to incompatible encodings.
  Rather than fixing this, remove JSON support in favor of only BSATN.
- Rewrite tests to use BSATN WS format.
- Also rewrite tests to be closer to legal in a few ways, but not entirely.
  E.g. don't put floats for `Point` fields with declared type `U16`.
- Hoist compression into a WS wrapper so that tests can bypass it.
- Update quickstart example for consistent filtering rules changes.
- Add `__identity_bytes`/`__address_bytes` getters to `Identity` and `Address`
  so serialization doesn't need to special-case them.
- Run prettier, which inflates this diff somewhat.
  If I had planned ahead I would have done this separately, but I didn't,
  and it's too late to separate the functional changes from the formatting now.

---------

Co-authored-by: Piotr Sarnacki <[email protected]>
@PuruVJ
Copy link
Collaborator Author

PuruVJ commented Jul 16, 2024

merge conflicts, nuking and going with PR train

@PuruVJ PuruVJ closed this Jul 16, 2024
@PuruVJ PuruVJ deleted the puru/modernize branch July 16, 2024 10:23
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.

3 participants