-
Notifications
You must be signed in to change notification settings - Fork 480
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
fix: type of Rule constructor and Rule's name #397
fix: type of Rule constructor and Rule's name #397
Conversation
As for the failure, this may shed some light. |
@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
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 And only then, when the current functionality exists in TypeScript, would I consider changing or extending it. How do you feel about this approach? |
@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. |
@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. |
@chris-pardy So do you agree with these changes? |
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, this is fine, it's going to get blown away by other changes but this is making the system better incrementally
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 ofRule.name
is technically breaking.