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

call Rule.ruleEvent by its correct name #404

Merged

Conversation

emilefokkemanavara
Copy link
Contributor

Currently, the types suggest that Rule has a property called event, but the property is actually called ruleEvent, as evidenced by this test. This PR fixes that by changing the type of Rule. This is not breaking because any TypeScript code referring to a Rule's event can never have worked.

@emilefokkemanavara
Copy link
Contributor Author

@chris-pardy What do you think of this?

@chris-pardy
Copy link
Collaborator

@emilefokkemanavara getting to this finally. What I'd prefer to do is add a property to the rule called event and then we can make this a non-breaking change both on the types and on the implementation.

@emilefokkemanavara
Copy link
Contributor Author

@chris-pardy But what I suggested above is already non-breaking, isn't it?

@emilefokkemanavara
Copy link
Contributor Author

@chris-pardy I implemented your suggestion. Rule now has both.

Copy link
Collaborator

@chris-pardy chris-pardy 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, I'll merge and deploy this later today.

@@ -175,7 +175,8 @@ export class Rule implements RuleProperties {
constructor(ruleProps: RuleProperties | string);
name: string;
conditions: TopLevelCondition;
event: Event;
ruleEvent: Event;
event: Event
Copy link
Collaborator

Choose a reason for hiding this comment

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

Last thing, we should mark one of these as deprecated.
No reason to remove it until a v8 but we should be starting that process.

Copy link
Contributor Author

@emilefokkemanavara emilefokkemanavara Dec 18, 2024

Choose a reason for hiding this comment

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

So we'll deprecate ruleEvent, right? (Because we want a Rule to still implement RuleOptions RuleProperties)

Copy link
Collaborator

Choose a reason for hiding this comment

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

correct.

@chris-pardy chris-pardy merged commit 2a1ec93 into CacheControl:master Dec 18, 2024
3 checks passed
@chris-pardy
Copy link
Collaborator

@emilefokkemanavara to add a note here on the reason your original change would have still been a breaking change.

There's effectively 2 artifacts being distributed as part of this library - types and code. Because of the nature of Typescript there's no restriction on how the types are used.

For instance say I have something like:

type RuleThings = Partial<Record<keyof Rule, string>>;

const ruleThings: RuleThings = {
   'event': 'test'
}

When we dropped the event property it would have broken this code.

Making the change the way we did we
a) make any code that was trusting the typescript types work
b) keep any existing code that uses the ruleEvent property working

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.

2 participants