Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

fix: type of Rule constructor and Rule's name#397

Merged
chris-pardy merged 2 commits intoCacheControl:v8 from
emilefokkemanavara:fix/rule-types
Nov 20, 2024
Merged

fix: type of Rule constructor and Rule's name #397
chris-pardy merged 2 commits intoCacheControl:v8 from
emilefokkemanavara:fix/rule-types

Conversation

@emilefokkemanavara
Copy link
Contributor

@emilefokkemanavara emilefokkemanavara commented Nov 16, 2024

Currently the types suggest that Rule needs an argument for its construction, but it doesn't. And when it is constructed without an argument, its name will be undefined. This PR adds a test to document this behavior and changes the types accordingly. It is targeted to v8 because the change to the type of Rule.name is technically breaking.

Copy link
Contributor Author

As for the failure, this may shed some light.

Copy link
Collaborator

@emilefokkemanavara thanks for putting in this PR. and thanks for all the contributions. I'm going to make a point of actually getting a contributors list going on this repo and acknowledging you (and everyone else)

The goal with the next major version is to do a bit of a ground-up rewrite in Typescript and keep the basic ideas and functionality but make it a lot more extensible. There's a couple different ways that could go but most likely it's turning Rules and Conditions into interfaces. Providing some standardized version of them but ultimately giving users of this library the option to roll-their own.

I'd love your thoughts and input on that work.

Copy link
Contributor Author

@emilefokkemanavara thanks for putting in this PR. and thanks for all the contributions. I'm going to make a point of actually getting a contributors list going on this repo and acknowledging you (and everyone else)

The goal with the next major version is to do a bit of a ground-up rewrite in Typescript and keep the basic ideas and functionality but make it a lot more extensible. There's a couple different ways that could go but most likely it's turning Rules and Conditions into interfaces. Providing some standardized version of them but ultimately giving users of this library the option to roll-their own.

I'd love your thoughts and input on that work.

@chris-pardy I saw your earlier attempt to do a TypeScript rewrite, and I thought I'd give it a shot as well. If only to get to know this project better. My approach was (and is) to

  • rename all mjs files to mts
  • add tsconfigs for the different parts (src, test, examples, types)
  • add types to the files in src and test until TypeScript no longer complains

All the while making sure the tests keep passing every step of the way. (It was while going through these steps that I came across the current issue.) Also, I'm not touching the types in types/index.d.ts, but rather importing them in the src files. This way I ensure that nothing changes functionally or in the API.

And only then, when the current functionality exists in TypeScript, would I consider changing or extending it. How do you feel about this approach?

Copy link
Collaborator

@emilefokkemanavara that was also my approach. I would say that the biggest "hurdles" to overcome with that is how not-typescripty some of the behavior in this library is.

For instance the Condition Result type you added is great but it's not really it's own type. In reality the library clones the condition classes and mutates them. Expressing this in Typescript is hard to say the least.

This is what is bringing me to the ground-up re-write approach. I think it's possible to do that with a very minimal amount of changes to the surface of the library itself, have complete backwards compatibility with rules, and provide codemods for the things that do need to change.

Copy link
Contributor Author

@chris-pardy So what's the status of your pr? Do you intend to keep working on that? What still needs to be done there to make the build pass? In any case, I would be happy to continue work on my step-by-very-small-step approach I outlined above.

Copy link
Collaborator

@chris-pardy So what's the status of your pr? Do you intend to keep working on that? What still needs to be done there to make the build pass? In any case, I would be happy to continue work on my step-by-very-small-step approach I outlined above.

I think it's just getting the build passing. If you'd like to run with that please feel free to do so.

Copy link
Contributor Author

@chris-pardy So do you agree with these changes?

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.

Yeah, this is fine, it's going to get blown away by other changes but this is making the system better incrementally

@chris-pardy chris-pardy merged commit e244b18 into CacheControl:v8 Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chris-pardy chris-pardy chris-pardy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /