-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
[code-infra] Use util from code-infra to fetch changelogs #47350
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
Conversation
brijeshb42
commented
Nov 20, 2025
- I have followed (at least) the PR section of the contributing guide.
Netlify deploy previewhttps://deploy-preview-47350--material-ui.netlify.app/ Bundle size report
|
093965c to
812f4a2
Compare
scripts/releaseChangelog.mjs
Outdated
| output: process.stdout, | ||
| }); | ||
| const startMessage = githubToken | ||
| ? `You have provided a GitHub token via --githubToken. It is not recommended to pass it now since we will deprecate this flag in the future.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to show a message, but further ignore anything user provided? e.e. "you've provided a deprecated --githubToken argument, this will be ignored."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can just remove this support since the Github app auth is anyway seemless and extra steps are required only the first time. Its used by us and not @mui/material users, so IMO, it should be fine to just ignore --githubToken or env var.
812f4a2 to
85301f6
Compare
|
The CI failing is because release:changelog also runs in CI and with the new flow, it doesn't work. So we'll have to support GITHUB_TOKEN env flow, at least in the core repo. |
c3d695b to
ee0df78
Compare
scripts/releaseChangelog.mjs
Outdated
| const bTags = parseTags(b.message); | ||
| if (aTags === bTags) { | ||
| return commitsItemsByDateDesc.indexOf(a) - commitsItemsByDateDesc.indexOf(b); | ||
| return commitsItemsByOrder.get(a) - commitsItemsByOrder.get(b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be reversed now if we don't reverse commitsItems anymore?
| return commitsItemsByOrder.get(a) - commitsItemsByOrder.get(b); | |
| return commitsItemsByOrder.get(b) - commitsItemsByOrder.get(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared with master I see a different ordering. Not sure what version is correct, but, the .reverse is removed, and the .get should return the same as the .indexOf from before. So if we don't inverse this subtraction, the order will be different than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not matching, e.g. check the "[code-infra] Enable Next.js ...." line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I was just looking at the start tags.
ee0df78 to
cc82117
Compare
| new Set( | ||
| commits | ||
| .filter((commit) => !!commit.author?.login) | ||
| .map((commit) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brijeshb42 nitpick, but any reason to use a different function style here vs.the above?
| .map((commit) => { | |
| .map((commit) => commit.author.login) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually i copied it over from Base UI's code.
