Skip to content

Refactor repo#138

Merged
jawj merged 33 commits intomainfrom
jawj/repo-reorg
Feb 11, 2025
Merged

Refactor repo#138
jawj merged 33 commits intomainfrom
jawj/repo-reorg

Conversation

@jawj
Copy link
Collaborator

@jawj jawj commented Jan 20, 2025

This PR makes some major changes, reorganising the repo into a more normal shape.

  • All source code is now found in src (previously most of it was split between export and shims)
  • Temporary build artefacts are in build, which is not committed to the repo
  • There's only one package.json, and it lives in the root folder
  • All built and bundled code that makes up the npm and JSR packages also lives in the root folder
  • The above makes it possible to npm install a specific commit or branch without special support.
  • Publishing to both npm and JSR is automated, with appropriate checks: just run npm version [patch|minor|major]

Note: I will make a follow-up PR to add printWidth: 100 to .prettierrc.

@jawj jawj requested a review from pffigueiredo January 28, 2025 10:45
@jawj jawj marked this pull request as ready for review January 28, 2025 10:47
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a compilation output, I thought we're removing those from being committed in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, and in an earlier .gitignore commit I did remove all build output. But then I thought: it's actually extremely useful to be able to npm install whichever commit or branch for testing and support purposes, right? @pffigueiredo even added a setup-release-branch.sh script to make this possible for specific commits, but that's no longer needed as long as we include the build output in the repo.

So — I put back the 4 index.* files that make up the exported package code. I also added a git pre-commit hook script to ensure that these files are up to date prior to any commit.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well there's 2 subjects here :)

  • The fact committing build output can be convenient because git essentially contains a superset of the NPM published package

    I guess it really can be convenient, but per-commit previews can be achieved by other means (definitely a lot more work though). One benefit to "doing it properly" is that installing a commit preview can be more similar to the eventually installed NPM tarball (for example some files may be excluded according to package.json but a commit preview with committed build output would still work)
    It's a trade-off, personally I'd pick having a cleaner git flow (without build output), and risk complicating a commit preview setup.

  • Whether a pre-commit is used to ensure it's up to date, instead of a CI check

    I really hate pre-commit hooks :D I'd always choose a CI check to VERIFY, and leave the responsibility of passing requirements to devs. I think commit/push hooks should never serve as a way to conform to something, only as a convenience factor (that can be opted in/out of, so optional).


Anyway just my own preference, it's a trade-off and it's ok if we just go with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the pre-commit hook for my own use (I've lost count of how many times I committed code that failed format checks!), but I agree we can't enforce that.

So I guess we could also check in CI that the built files are all up to date?

Copy link
Contributor

@amitdahan amitdahan left a comment

Choose a reason for hiding this comment

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

lgtm! let's wait for @pffigueiredo 's review too since this is a big change

@pffigueiredo
Copy link
Contributor

I'm not the biggest fan of having index.d.mts and related right in the root folder, but it's also not something I'm willing to die on a hill for 👍

@jawj jawj merged commit 6bf306b into main Feb 11, 2025
6 checks passed
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