Add new rule parser package #65171
Add new rule parser package #65171
Conversation
senadir
added
the
[Type] New API
senadir
requested review from
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @joshuatf. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +1.25 kB (+0.07%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
senadir
force-pushed
the
add/rule-parser-package
branch
from
2f3e2ac
to
04c17ff
Compare
There was a problem hiding this comment.
Fantastic work, @senadir! Love where this is going. It might be easier to review if we broke this PR into smaller chunks, but I left some high level thoughts throughout the PR.
I'm still torn between handling this at package level (by introducing a hook that allows providing dependencies) or leaving it up for implementors.
I think leaving this up to consumers sounds like a good strategy. In React, it's simple enough to throw the parser into a useEffect. Other consumers may require different implementations, so leaving this package simple works well IMO.
I don't have strong feelings about this, but I consider writing JSON objects with the same keys a bit too verbose for this goal, given how well defined the array shape is.
I agree. In an ideal world, we could keep everything in JSON Schema, but this feels overkill for this package and we can leave that responsibility to the validation package.
| ]; | ||
| ``` | ||
|
|
||
| 4. Nested arrays |
There was a problem hiding this comment.
Nice! Thanks for considering these cases.
| [ | ||
| 'ALL', | ||
| [ | ||
| [ 'user.id', 'in', [ 1, 2, 3 ] ], |
There was a problem hiding this comment.
The dot notation makes me think we'll be able to access nested properties. For example, user.id would access the property here:
{
"user": {
"id": 5
}
}But looking at the sample data below, it looks like that might not be the case. Do we want to support dot notation or will consumers be responsible for flattening all their data?
There was a problem hiding this comment.
The very initial implementation we had at Checkout operated that way, this added the need to parse things. I think we might want to provide even greater flexibility to consumers, so instead of passing an object of key value, they can also pass an object of keys and functions/loaders, or receive a single loader function and they decide what to do with it.
For now we can probably settle on supporting dot notation out of the box?
| ); | ||
| } ); | ||
|
|
||
| it( 'should parse with newly introduced evaluator', () => { |
There was a problem hiding this comment.
Should we also have negative test cases around the newly introduced evaluator?
| @@ -0,0 +1,155 @@ | |||
| /** | |||
There was a problem hiding this comment.
Should we add some rules around thrown errors from data sources where inappropriate types are passed?
| return source >= min && source <= max; | ||
| } ); | ||
|
|
||
| registry.alias( '<>', 'between' ); |
There was a problem hiding this comment.
Love the alias option! ❤️
| source: Source, | ||
| target: Target, | ||
| rule: Rule, | ||
| strict: boolean = true |
There was a problem hiding this comment.
The name strict is throwing me a little bit. Maybe exclusive or inclusive?
youknowriad
requested review from
a team,
getdave and
cbravobernal
and removed request for
a team
| export type Value = string | number | boolean; | ||
| export type Source = Value | Value[]; | ||
| export type Target = Value | Value[]; | ||
| export type Operator = string; |
There was a problem hiding this comment.
Is it possible to type it as constants so that developers can get autocompletion?
There was a problem hiding this comment.
I'm not I understand this, can you provide an example of what this should change to?
There was a problem hiding this comment.
I believe the question is that rather than string can this be typed as a union of string literals: 'in' | '!in' | 'is' | ….
There was a problem hiding this comment.
Ah yeah! It was originally like that, the issue I run into is that once you have an extensible registry that allows additional operators, the type would fail.
I suppose we can add a list of possible operators to the parser< T > call?
There was a problem hiding this comment.
I suppose the type can be
| export type Operator = string; | |
| export type Operator = | |
| | 'is' | |
| | '=' | |
| | 'not is' | |
| | '!=' | |
| | 'contains' | |
| | 'not contains' | |
| | '!contains' | |
| | 'in' | |
| | 'not in' | |
| | '!in' | |
| | 'greater than' | |
| | '>' | |
| | 'less than' | |
| | '<' | |
| | 'gte' | |
| | '>=' | |
| | 'lte' | |
| | '<=' | |
| | string; |
|
This looks a bit like a lisp variant, I could imagine the syntax being like written like this in a lisp language: (every (list
(contains post-categories "tutorials")
(is user-role 'editor')
))note: I haven't written lisp in ages, so please don't criticize this code snippet too much! I haven't really thought too deeply about the purpose of this, but focusing on the syntax and borrowing from Lisp, a suggestion would moving the function/operator to be a prefix so that there's plenty of future flexibility. |
|
Thanks for the PR!
Can you provide some more details how this package will help core? Will existing code be simplified? Do we have many needs for such code? |
|
Hey @ntsekouras!
There's no current code that we can rewrite to benefit from this, we're kinda jumping the gun here and working on parallel, the team working on DataForms will most definitely need that, and we (downstream consumers) also need that, so there's benefit from standardizing on this early on. When I talked with Matias about this, he did express interest in bringing Blocks Visibility functionality eventually into core. so TL;DR: there's no direct usage, this package will sit, and be actually used outside Gutenberg first and inside Gutenberg in the coming months. |
|
Thanks for the feedback @talldan! would this mean switching to |
Yep 👍 . I saw the argument for keeping it at 3 items in the PR description, but I still think it might be good to leave it open to more arguments as a future possibility, or even support singular arguments (e.g. |
In that case, why not create a completely different npm package (outside of WP), start using it and test it as 3rd party consumers. If everything works well, we can port to WP core to have the benefits of something used in core and needed from others. If it doesn't work well, you will be able to adjust with any breaking changes you need and then port. Introducing packages and public APIs in WP are a big burden, as we should always be backwards compatible. We should be very careful and intentional these matters. --cc @WordPress/gutenberg-core |
|
Thanks for the feedback Nik! I will explore that. We do have validated need for this in Gutenberg, though it's a bit early. I totally understand the concern of expanding the public area, which is why I wanted to only expose this via npm first, so that we can iterate on any breaking changes in a single place first. Would that still have the same risk? |
senadir
force-pushed
the
add/rule-parser-package
branch
from
23281cb
to
4e0e9ff
Compare
I was a bit confused by this verbiage. Does "implementors" here mean something like "platform developers", and "direct developer" a "third-party developer" (an "extender" in WP terminology)? |
Hey Lena! I can probably do a better job of explaining this, but your explaining is exactly correct. This package is simply an underlying tool (this is a similar one I discovered yesterday). An Implementor would be Gutenberg or WooCommerce, they would be responsible for collecting rules from their end-developers/extenders, they would provide the context (cart.cartTotals = 150), they would execute the parser, and they would take that value and run with it. I will adopt extenders, but looking for something to replace Implementors. |
|
Hey, interesting work here! Why do you want this package to live in the Gutenberg repository? It does a lot of things up-front without solving (even a minimal) use case in core. We haven't added unused packages before, and I am reluctant to start doing it. It's a difficult line to walk. How do we decide what's going to be used and what not based on any plugin's roadmap? I'd be happy to have this here if it's applied to a concrete use case in core. If what you envision is not ready yet in core, perhaps it can live elsewhere ( |
|
I think there are a couple of questions we need to answer here: 1. Does core have a need for this? I'm certain that downstream consumers of DataForms will need a way to conditionally hide or show fields in an extensible way, but I'm not certain whether or not this immediate need exists in core yet. @youknowriad @oandregal Will block settings be replaced by DataForms? If so, image block settings might be an example of an area that could benefit from this type of parser. (See "Scale" is shown/hidden depending on "Aspect Ratio" selection). 
2. Is there enough crossover with a validation library that would create redundancies across packages? This question was brought up by @dmsnell and I think is worth revisiting. Core has a more immediate need for a data validation library and JSON Schema seems like a viable approach to a validator package. Below are the above examples translated to JSON Schema. // const rule = [
// [ 'user.role', 'is', 'editor' ]
// ];
{
"$schema": "https://json-schema.org/draft/2019-09/schema",
"type": "object",
"properties": {
"role": {
"const": "editor"
}
}
}
// const rules = [
// [ 'user.role', 'is', 'editor' ],
// [ 'post.categories', 'contain', 'tutorials' ]
// ];
{
"$schema": "https://json-schema.org/draft/2019-09/schema",
"type": "object",
"properties": {
"role": {
"const": "editor"
},
"categories": {
"type": "array",
"contains": {
"const": "tutorials"
}
}
}
}
// const rules = [
// 'ANY',
// [
// [ 'user.role', 'is', 'editor' ],
// [ 'post.categories', 'contain', 'tutorials' ],
// [
// 'ALL',
// [
// [ 'user.id', 'in', [ 1, 2, 3 ] ],
// [ 'post.blocks', 'not contain', 'core/embed' ]
// ]
// ]
// ]
// ];
{
"$schema": "https://json-schema.org/draft/2019-09/schema",
"type": "object",
"anyOf": [
{
"properties": {
"role": {
"const": "editor"
},
}
},
{
"properties": {
"categories": {
"type": "array",
"contains": {
"const": "tutorials"
}
}
}
},
{
"properties": {
"id": {
"type": "number",
"enum": [ 1, 2, 3 ]
},
"blocks": {
"not": {
"contains": {
"const": "core/embed"
}
}
}
}
}
]
}This is undoubtedly more verbose, but much more declarative and JSON Schema is well documented with a number of pre-existing tools that we can use. If we opt to use JSON Schema, we can combine the efforts here and validation into a single package and focus on creating a package that core has a more immediate use for. |
|
The goal of pushing right now is to agree on a shape that we can confidently adopt in WooCommerce, and have the assurance that we're not introducing yet another solution for the same problem. It's a problem we know Gutenberg will need solving soon, just not yet. Given this is going to be a public API for us, it's hard to ask developer to use this rule shape, then several months later, Gutenberg ship the same thing, but using a different schema, and we now have 2 ways to do things, and have diverged from core. @oandregal @ntsekouras Ignoring the code (happy to close this one right now, as for me, this is a means to an end): is it possible to adopt a style guide for how rules should be written? Without shipping code for it? I understand this is a hard question to answer without actually putting it for use? But if we can do, and you point me to where to go, I would be happy to move away for this, get a proposal in place, and adopt it in WooCommerce. |
|
Thank you everyone for the feedback, I highly appreciate the candid responses here. I'm going to close this package and follow up with a new PR that introduces validation and conditional visibility, based on JSONSchema. We will have direct use cases for this inside the DataViews/DataForms work, and likely to ship it there first before breaking out to its own package. Let me know if you want to be involved in reviewing/approving that PR. |
What?
This PR introduces a new package
@wordpress/rule-parserthat's meant to evaluate logical, array based rules, into a boolean.Why?
The main usage for this package would be to solve a couple of problems:
The API
Currently, this API only covers the JS side, but a following PHP should be added once we settle on how this one works, a big requirement for this is that it developers write the logic once, and that evaluates to the same value in both JS and PHP.
Note
This package is only meant to be used by implementors, not direct developers. implementors are then responsible for collecting rules from developers or users, either via UI (like Blocks Visibility), or code (like DataForms and Checkout), evalute the rules against their provided context, and use the outcome however they see fit.
The package accepts rules in the shape of
[ source, operator, target ]and can be infinitely nested on a variation of combinators, that resemble||(OR, ANY) or&&(AND, ALL).Examples of rules
ALLwill be used.Anatomy of a rule
A rule is made of 3 values, a source, an operator, and a target.
Along of combinators, an array of rules can be transformed to a single boolean.
Usage
Context doesn't have to be static and predefined, and I think we can also functions that gets called at evaluation time.
Reactivity
The package currently isn't reactive, it means you need to recall parser each time context changes up, I'm still torn between handling this at package level (by introducing a hook that allows providing dependencies) or leaving it up for implementors.
Operators
The package ships with a simple registry that allows you register a new operator or an alias to an operator. This should allow implementor to specify which rules they want to support, and even add additional ones that aren't covered by core.
Why Arrays and not other types of logic.
Before reaching this shape, I evaluated several options, I will go over them here:
Writing regular functions
One option would be to write a regular PHP and JS functions that just evaluates to a boolean, skipping the need to go over an abstraction that eventually does that for you.
The main issue with that is portability and duplication of logic. For developers, they will need to write the same logic twice, which is in itself not a huge issue. But the main issue is portability, for us, we want developers to have their logic apply for the current website, and any external site consuming the API, including external Checkout services, and you can't send or trust JS over the wire.
Writing eval-able pseudo code.
This option involves writing JS code that gets evaluated by a custom lexical parser. WooCommerce new product editor uses (described in this
@woocommerce/expression-evaluationpackage).The benefit of this approach is that you write a single string instead of arrays, but the drawback is the need to have 2 parsers (in JS and PHP), which is in itself not a huge lift.
The main concern is simply that lack of already structured arrays reduces the ability to better type and validate them.
Internally in Woo, and after some discussions, we decided this is not the approach we want to continue with.
N-item arrays
Block Visibility plugin uses a similar approach, but instead of having 3 items arrays, it can have several items, being for example a general selector
postand a subSelector ofpostId.I initially considered this approach but decided it's a bit unpredictable for implementor and authors to know, and opted to have dot notation, thou not forced. Implementor can decide how their system translates back to this notation.
JSON objects
Going the same route as above, but instead of 3 items arrays, you use JSON objects to describe the rule, this is the route that WooCommerce's remote notification system works (by sending a note and a collection of rules with it to decide if it should be shown or not), described in this document.
I don't have strong feelings about this, but I consider writing JSON objects with the same keys a bit too verbose for this goal, given how well defined the array shape is.
Next steps and remaining tasks
I'm tagging some people who can help review this and validate the approach.
@youknowriad @oandregal @ndiego @joshuatf