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

Make commit information opt-in #107

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Make commit information opt-in #107

wants to merge 13 commits into from

Conversation

Jym77
Copy link
Contributor

@Jym77 Jym77 commented Nov 22, 2024

Previously, we were automatically running simple-git to extract some commit information, and send it (with possible opt-out).

This causes a major problem that simple-git relies on running in a Node environment, thus simply trying to bundle SIP.ts (which greedily bundles simple-git) for a browser environment with Webpack or the likes was causing the bundler to crash at build time.
There was a minor inconvenience that the naming was tied to git, even if it represents nearly 90% of usage, that was a bit exclusive of other versioning systems.


With this PR, the commit information is optional and has to be provided by the caller. This should make SIP.upload usable in more environments. A getCommitInformation helper is still provided for git, but now has to be explicitly included by the user, thus not crashing the bundling in unexpected and hard-to-track ways.

CommitInformation.GitOrigin has also been renamed CommitInformation.Origin for better compatibility.

Copy link

changeset-bot bot commented Nov 22, 2024

🦋 Changeset detected

Latest commit: ea299ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 27 packages
Name Type
@siteimprove/alfa-test-utils Minor
@siteimprove/alfa-angular Minor
@siteimprove/alfa-assert Minor
@siteimprove/alfa-chai Minor
@siteimprove/alfa-cheerio Minor
@siteimprove/alfa-cli Minor
@siteimprove/alfa-command Minor
@siteimprove/alfa-crawler Minor
@siteimprove/alfa-cypress Minor
@siteimprove/alfa-enzyme Minor
@siteimprove/alfa-formatter-earl Minor
@siteimprove/alfa-formatter-json Minor
@siteimprove/alfa-formatter-sarif Minor
@siteimprove/alfa-formatter Minor
@siteimprove/alfa-frontier Minor
@siteimprove/alfa-interviewer Minor
@siteimprove/alfa-jasmine Minor
@siteimprove/alfa-jest Minor
@siteimprove/alfa-jquery Minor
@siteimprove/alfa-playwright Minor
@siteimprove/alfa-puppeteer Minor
@siteimprove/alfa-react Minor
@siteimprove/alfa-scraper Minor
@siteimprove/alfa-selenium Minor
@siteimprove/alfa-unexpected Minor
@siteimprove/alfa-vue Minor
@siteimprove/alfa-webdriver Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Jym77 Jym77 changed the title Extract git Make commit information opt-in Nov 22, 2024
@Jym77 Jym77 self-assigned this Nov 22, 2024
@Jym77
Copy link
Contributor Author

Jym77 commented Nov 22, 2024

!pr extract

@Jym77 Jym77 added the major Backwards-incompatible change that touches public API label Nov 22, 2024
@Jym77 Jym77 marked this pull request as ready for review November 22, 2024 15:22
@Jym77 Jym77 requested a review from a team as a code owner November 22, 2024 15:22
@Jym77
Copy link
Contributor Author

Jym77 commented Nov 22, 2024

@jfer-siteimprove FYI.
Note that this changes the name of CommitInformation.GitOrigin to the more generic CommitInformation.Origin which will be a breaking change on your side 🙈

Copy link
Contributor

@rcj-siteimprove rcj-siteimprove left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Backwards-incompatible change that touches public API
Projects
Status: 🧑‍💻 Doing
Development

Successfully merging this pull request may close these issues.

2 participants