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

lib: make event static properties non writable and configurable #50425

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

BenzeneAlcohol
Copy link
Contributor

The idl definition for Event makes the properties constants, this means that they shouldn't be configurable. However, they were, and this commit fixes that.

Fixes: #50417

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 27, 2023
@BenzeneAlcohol
Copy link
Contributor Author

making writable: false, makes it such that the user cannot even assign a value. Is that what we intend at the end?

@BenzeneAlcohol
Copy link
Contributor Author

I've run the linter and fixed the errors, and also changed up stuff a bit.

Please review @KhafraDev

@H4ad H4ad requested a review from KhafraDev October 30, 2023 15:36
@H4ad H4ad added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2023
@H4ad
Copy link
Member

H4ad commented Oct 30, 2023

@BenzeneAlcohol Can you please fix the issue on the first commit?

https://github.com/nodejs/node/actions/runs/6690083519/job/18190085079?pr=50425

But thanks for the PR, looks great!

@BenzeneAlcohol
Copy link
Contributor Author

@BenzeneAlcohol Can you please fix the issue on the first commit?

https://github.com/nodejs/node/actions/runs/6690083519/job/18190085079?pr=50425

But thanks for the PR, looks great!

Done, thank you! :)

@BenzeneAlcohol
Copy link
Contributor Author

The test for some reason runs on the old commit message?
Do I have to squash all the commits into one manually?

@BenzeneAlcohol
Copy link
Contributor Author

Any changes I have to further make?

@H4ad
Copy link
Member

H4ad commented Oct 31, 2023

You can squash everything, or you can rebase, but you need to change the commit message, both should work.

@BenzeneAlcohol
Copy link
Contributor Author

Squashed into one commit.

lib/internal/event_target.js Outdated Show resolved Hide resolved
test/parallel/test-event-target.js Show resolved Hide resolved
@BenzeneAlcohol
Copy link
Contributor Author

Ok, I have a very noob question.

But I do some small changes, and every time do I have to make the detailed commit message? Because sometimes the change I do is like removing a few lines (like I removed a few console lines) but then the commit message should contain details about the actual change in case it gets merged.

So do I just wait for approval, then rebase all changes to 1 commit message? And have normal commit messages, 1 liners till that one final rebased single commit.

lib/internal/event_target.js Outdated Show resolved Hide resolved
@H4ad
Copy link
Member

H4ad commented Nov 1, 2023

From what I know, only the first message will be used to create the commit description, so it's fine you keep the first commit with the message and then create other commits for minor fixes.

About all commits, I will later add a flag to squash everything, so all the commits will be released as just one commit.

@KhafraDev
Copy link
Member

But I do some small changes, and every time do I have to make the detailed commit message? Because sometimes the change I do is like removing a few lines (like I removed a few console lines) but then the commit message should contain details about the actual change in case it gets merged.

I believe only the first commit must be detailed, because they typically get squashed automatically when merged.

Personally, I use git rebase -i HEAD~[number of commits] and then squash all the commits together into the first one, then git push -f. That way I only need to worry about a single commit.

@BenzeneAlcohol
Copy link
Contributor Author

But I do some small changes, and every time do I have to make the detailed commit message? Because sometimes the change I do is like removing a few lines (like I removed a few console lines) but then the commit message should contain details about the actual change in case it gets merged.

I believe only the first commit must be detailed, because they typically get squashed automatically when merged.

Personally, I use git rebase -i HEAD~[number of commits] and then squash all the commits together into the first one, then git push -f. That way I only need to worry about a single commit.

Got it, thanks!

@BenzeneAlcohol
Copy link
Contributor Author

From what I know, only the first message will be used to create the commit description, so it's fine you keep the first commit with the message and then create other commits for minor fixes.

About all commits, I will later add a flag to squash everything, so all the commits will be released as just one commit.

Alrighty!

@H4ad H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2023
@nodejs-github-bot
Copy link
Collaborator

@BenzeneAlcohol
Copy link
Contributor Author

Does that failed pipeline for 4 checks indicate any changes I have to do or are they some flaky tests?

@MrJithil
Copy link
Member

MrJithil commented Nov 2, 2023

Its not because of your changes.

@nodejs-github-bot
Copy link
Collaborator

@BenzeneAlcohol
Copy link
Contributor Author

Ok, I've made the changes hopefully the pipeline runs successfully 👀

Please look into the pending comment too

Done

Copy link
Member

@MrJithil MrJithil left a comment

Choose a reason for hiding this comment

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

LGTM

@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2023
@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen changed the title lib: Event static properties non configurable and writable lib: make event static properties non writable and configurable Nov 3, 2023
@tniessen
Copy link
Member

tniessen commented Nov 3, 2023

Is this semver-major or do we consider it a bug fix?

@H4ad
Copy link
Member

H4ad commented Nov 3, 2023

I think it is a bug fix since people are not supposed to change/modify these properties.

But if we want to be cautious, a semver-minor can be applied.

Maybe run CITMG too?

@KhafraDev
Copy link
Member

KhafraDev commented Nov 3, 2023

definitely a bug fix, I wouldn't consider it a major change. If someone relied on changing these properties, they were relying on a bug.

@BenzeneAlcohol
Copy link
Contributor Author

definitely a bug fix, I wouldn't consider it a major change. If someone relied on changing these properties, they were relying on a bug.

Agreed. Does it need semver-minor?

@BenzeneAlcohol
Copy link
Contributor Author

Does this need anything else before merging?

@BenzeneAlcohol
Copy link
Contributor Author

@KhafraDev and @H4ad any blockers stopping this from merge?

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2023
@nodejs-github-bot nodejs-github-bot merged commit b4850f2 into nodejs:main Nov 10, 2023
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b4850f2

@MrJithil
Copy link
Member

MrJithil commented Nov 10, 2023

@KhafraDev and @H4ad any blockers stopping this from merge?

IMO No. It's already landed

targos pushed a commit that referenced this pull request Nov 11, 2023
The idl definition for Event makes the properties constant
this means that they shouldn't be configurable and writable.
However, they were, and this commit fixes that.

Fixes: #50417
PR-URL: #50425
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
targos pushed a commit that referenced this pull request Nov 14, 2023
The idl definition for Event makes the properties constant
this means that they shouldn't be configurable and writable.
However, they were, and this commit fixes that.

Fixes: #50417
PR-URL: #50425
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
The idl definition for Event makes the properties constant
this means that they shouldn't be configurable and writable.
However, they were, and this commit fixes that.

Fixes: #50417
PR-URL: #50425
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

events: static properties are configurable & writable
6 participants