Update Commit config to separate add/version and add back skipCi by rohit-gohri · Pull Request #735 · changesets/changesets

rohit-gohri · 2022-01-13T05:37:43Z

PoC for a possible solution for #688. Made commit option configurable separately for add/version commands. Adds back skipCI option

{
  "commit": true
}
// above evaluates to below
{
  "commit": {
    "add": {
      "skipCI": false
    },
    "version": {
      "skipCI": true
    }
  }
}

Other ways:

// only add
{
  "commit": {
    "add": {
      "skipCI": false
    },
    "version":false
  }
}
// only version
{
  "commit": {
    "add": false,
    "version": {
      "skipCI": false
    }
  }
}

changeset-bot · 2022-01-13T05:37:46Z

🦋 Changeset detected

Latest commit: c6c0533

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

This PR includes changesets to release 4 packages

Name	Type
@changesets/apply-release-plan	Minor
@changesets/cli	Minor
@changesets/config	Minor
@changesets/types	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

codesandbox-ci · 2022-01-13T05:38:07Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c6c0533:

Sandbox	Source
Vanilla	Configuration

Andarist · 2022-01-15T17:07:43Z

Thank you for the PR - I'll try to do a review as soon as possible but I have lots on my plate right now and can't get to everything right away (even though I would really like to 😛 )

Andarist · 2022-01-23T22:51:50Z

I see how this solution is more robust but I'm not yet sure if we want to allow people to hook into such stuff. I think that for now I would prefer if we'd go with a simpler, builtin, solution.

I imagine that we could support this:

{
  "commit": {
    "version": {
      "skipCI": true
    },
    "publish": {
      "skipCI": true
    }
  }
}

We'd have to make sure that we use the same default values as the current, implicit, ones. A config like this should only enable auto-commits for the version command:

{
  "commit": {
    "version": true
  }
}

rohit-gohri · 2022-01-28T19:41:42Z

@Andarist Have updated the PR to an approach similar to what you described

Andarist · 2022-01-29T10:42:46Z

Thank you for the PR - I will try to review it after the weekend

Andarist · 2022-02-12T09:25:19Z

packages/types/src/index.ts

 export type Config = {
  changelog: false | readonly [string, any];
-  commit: boolean;
+  commit: Commit;


This is a breaking change for all packages that receive config from the outside, such as @changesets/apply-release-plan. Previously they could assume that this is a boolean. I think it's not worth a breaking change and perhaps we don't have to normalize this (at least not for the time being) to this object shape.

Andarist · 2022-02-12T09:30:31Z

packages/config/src/index.ts

  packages: [fakePackage]
 });
+
+export { getAddLine, getVersionLine };


I think that those don't belong in this package, at least for now - moving those should be reverted, they should stay declared closer to where they are actually called.

Andarist · 2022-02-12T09:32:17Z

packages/config/src/index.ts

-      )} when the only valid values are undefined or a boolean`
+      )} when the only valid values are undefined or a boolean or an object`
    );
+  } else if (typeof json.commit === "object") {


we could also add a check if skipCI is of the correct type within this block

Andarist · 2022-02-12T09:37:20Z

packages/config/package.json

    "fs-extra": "^7.0.1",
-    "micromatch": "^4.0.2"
+    "micromatch": "^4.0.2",
+    "outdent": "^0.5.0"


was this added as a runtime dependency? perhaps this should be moved to a dev dep or removed altogether when this suggestion gets applied

Andarist · 2022-02-12T10:00:40Z

Actually… while taking a shower i was thinking about this more 😅and i think i’ve figured out an alternative, more robust, design. Gonna post it later today so maybe let’s hold working on this PR until i write it down

Andarist · 2022-02-12T23:55:29Z

My proposal is to accept a string for the commit option that would point to a module responsible for generating those commit messages. We already do a similar thing with "changelog generators":

changesets/packages/apply-release-plan/src/index.ts

Lines 201 to 223 in c2b9d44

    
           let getChangelogFuncs: ChangelogFunctions = { 
        
             getReleaseLine: () => Promise.resolve(""), 
        
             getDependencyReleaseLine: () => Promise.resolve("") 
        
           }; 
        
           let changelogOpts: any; 
        
           if (config.changelog) { 
        
             changelogOpts = config.changelog[1]; 
        
             let changesetPath = path.join(cwd, ".changeset"); 
        
             let changelogPath = resolveFrom(changesetPath, config.changelog[0]); 
        
             let possibleChangelogFunc = require(changelogPath); 
        
             if (possibleChangelogFunc.default) { 
        
               possibleChangelogFunc = possibleChangelogFunc.default; 
        
             } 
        
             if ( 
        
               typeof possibleChangelogFunc.getReleaseLine === "function" && 
        
               typeof possibleChangelogFunc.getDependencyReleaseLine === "function" 
        
             ) { 
        
               getChangelogFuncs = possibleChangelogFunc; 
        
             } else { 
        
               throw new Error("Could not resolve changelog generation functions"); 
        
             } 
        
           }

I feel like this would be the most robust option that wouldn't require us to add a somewhat weird shape of the commit config option. This way the default module for this (that probably should be just an additional entry in the @changesets/cli) could not add [skip ci] at all but it would be super easy to create a module that would call the default one and just adjust the return value slightly. I do a similar thing with the default changelog generator here:
https://github.com/contentlayerdev/contentlayer/blob/7c06f352e73e265ad57e0ef12c88b892bc85837d/scripts/changeset-changelog-generator.js

Note that when using the default "commit generator" we'd have to add [skip ci] to the message associated with one of the commands to maintain backward compatibility. This could happen in the code and outside of this default module though - unless this would be too cumbersome to pull off in the codebase, then it's just fine to include that in the default module.

Open questions:

what arguments function(s) exposed by this module should receive?
should the module expose a single function or two? it's important to know the content of which command we are generating but this information could be given in an argument, so using a single function is an option. I didn't think through which one should be preferred here though.

rohit-gohri · 2022-02-13T16:38:25Z

My proposal is to accept a string for the commit option that would point to a module responsible for generating those commit messages.

Ha, that was the orignal PR's approach. It's still in this old branch: main...rohit-gohri:old
I should have just opened a new PR 🤦 but I force-pushed instead to keep the discussion in single place.

Though I didn't move it into it's own module, I'll open a new PR with the changes.

what arguments function(s) exposed by this module should receive?

the changesets for the add and the release info for the publish, and any config?

should the module expose a single function or two?

I think 2 makes more sense as the input arguments would be completely different.

Andarist · 2022-02-13T19:45:24Z

Ha, that was the orignal PR's approach. It's still in this old branch: main...rohit-gohri:old

Heh, ye - I've reconsidered my previous opinion after getting a better look at what the alternative actually looks like 😅

Though I didn't move it into it's own module

It doesn't have to be a new module - an additional exntrypoint in the @changesets/cli should be sufficient. Take a look at how the one for the changelog is being defined here:

changesets/packages/cli/package.json

Line 26 in 0d1debd

"changelog"

I'll open a new PR with the changes.

Feel free to force-push to this one if you prefer that.

the changesets for the add and the release info for the publish, and any config?

Yep, sounds alright.

I think 2 makes more sense as the input arguments would be completely different.

Agreed. Such functions could actually be typed somewhat OK with the upcoming dependent parameters but ye, it's probably more straightforward to use separate functions.

rohit-gohri · 2022-03-05T09:00:49Z

Opened #768 based on the discussion here, didn't force push so that we can compare the approaches. Closing this.

rohit-gohri mentioned this pull request Jan 13, 2022

Ability to set commit to different values in add and version commands. #688

Closed

rohit-gohri changed the title ~~Add CommitFunctions and update config with defaults~~ Update Commit config to separate add/version and add back skipCi Jan 28, 2022

rohit-gohri added 2 commits January 29, 2022 11:25

Update Commit config to separate add/version and add back skipCi

f5c3d6c

Add changeset

c6c0533

Andarist reviewed Feb 12, 2022

View reviewed changes

rohit-gohri mentioned this pull request Mar 5, 2022

Add CommitFunctions and update config with defaults #768

Merged

rohit-gohri closed this Mar 5, 2022

Conversation

rohit-gohri commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codesandbox-ci bot commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andarist commented Jan 15, 2022

Uh oh!

Andarist commented Jan 23, 2022

Uh oh!

rohit-gohri commented Jan 28, 2022

Uh oh!

Andarist commented Jan 29, 2022

Uh oh!

Andarist Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

Andarist Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

Andarist Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

Andarist Feb 12, 2022

Choose a reason for hiding this comment

Uh oh!

Andarist commented Feb 12, 2022

Uh oh!

Andarist commented Feb 12, 2022

Uh oh!

rohit-gohri commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andarist commented Feb 13, 2022

Uh oh!

rohit-gohri commented Mar 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rohit-gohri commented Jan 13, 2022 •

edited

Loading

changeset-bot bot commented Jan 13, 2022 •

edited

Loading

codesandbox-ci bot commented Jan 13, 2022 •

edited

Loading

rohit-gohri commented Feb 13, 2022 •

edited

Loading