Declare static examples using namedParams #2308
Conversation
paulmelnikow
added
service-badge
Generated by 🚫 dangerJS |
|
Have labeled this a blocker as it blocks future work on the frontend including a branch where I've started refactoring toward #701, and future work on BaseService. |
services/base.js
Outdated
| this.name | ||
| } at index ${index} declares both namedParams and exampleUrl` | ||
| ) | ||
| } else if (!namedParams && !exampleUrl) { |
There was a problem hiding this comment.
Do we also want a check here that if you've declared namedParams we also need a pattern (or at least the URL shouldn't be defined using a regex)? Also prepareExamples() seems to have become quite large and unwieldy. I think we could usefully split all these validation rules out into their own function. In general we are now supporting quite a lot of different ways of doing the same thing (for valid reasons). It would be nice to be able to simplify this at some point in future, but I realise there is more refactoring work that must be done before we can simplify this.
There was a problem hiding this comment.
We can't get here if there's no pattern since we throw an error a few lines up. The only case we don't check is previewUrl + namedParams + !staticExample. At some point this stuff just needs to be rewritten using a Joi schema, though I expect the error messages will be a bit less friendly. Totally agreed it would be nice to consolidate ways of doing things. That said, this work is a lot more manageable if it's done in backward-compatible ways. Hopefully we can keep pushing the rewrite forward and move toward things being consistent.
I started a refactor of the badge preparation, which is basically keeping the example declarations as close as possible to what the frontend gets. In that branch there's only one transform to do here, and the only other responsibility of this function is validation. I'd be happy to pick up the validation refactor as part of that follow-on work. I tried to keep these changes incremental.
There was a problem hiding this comment.
We can't get here if there's no pattern since we throw an error a few lines up.
👍
That said, this work is a lot more manageable if it's done in backward-compatible ways
Yes agreed. As I say, it is complicated for valid reasons. It will just be nice when they stop being valid :)
Regardless of future plans, what do you think of splitting the if statement/exception throwing block out into a function (maybe validateExample()? ) just so that prepareExamples() is doing less stuff. Even if we change from a bunch of if statements to a Joi schema at a later date, that's still a sensible seperation of concerns.
There was a problem hiding this comment.
Yes agreed. As I say, it is complicated for valid reasons. It will just be nice when they stop being valid :)
😁
Regardless of future plans, what do you think of splitting the
ifstatement/exception throwing block out into a function (maybevalidateExample()? )
Okay. You wore me down. 😉
| namedParams: { world: 'World' }, | ||
| staticExample: this.render({ namedParamA: 'foo', queryParamA: 'bar' }), | ||
| keywords: ['hello'], | ||
| }, |
There was a problem hiding this comment.
As the validation logic grows more complicated its all got a bit confusing it would be helpful to formalise the behaviour with some tests in here ensuring we're doing the right thing with various combinations of previewUrl / exampleUrl / namedParams etc. Moving the validation rules to their own function will also help with this.
There was a problem hiding this comment.
Do you mean tests of invalid examples? Yea, agreed.
services/base.js
Outdated
| urlPattern, | ||
| staticExample, | ||
| documentation, | ||
| keywords, | ||
| }, | ||
| index | ||
| ) => { | ||
| pattern = pattern || urlPattern || this.route.pattern |
There was a problem hiding this comment.
I'm confused about why we want both pattern and urlPattern and what situation we use each one in. Can you explain the difference.
There was a problem hiding this comment.
We wouldn't want both. It's a temporary alias to pattern to make this diff smaller. I added it to the docstring. Does that make sense?
There was a problem hiding this comment.
Does that make sense?
Maaaybe.. Am I right that essentially what you really want to do is rename urlPattern to pattern, but we're supporting both so that we don't have to change every single badge that declares urlPattern to pattern in this PR.. but then we want to do that at a later date and then remove urlPattern when we've done that?
There was a problem hiding this comment.
Right; rename urlPattern to pattern, while also defaulting it to route.pattern whenever that's been declared.
It seems confusing to call the same / same kind of string pattern within route, and urlPattern within examples.
There was a problem hiding this comment.
I'm quite keen that we should have tests for this logic so I've written them and pushed a couple of additional commits to your branch - does this look OK? If so, I think this is fine to merge as it is but we should aim to remove the existing uses of urlPattern in the near future so we can remove that fallback from the base.
|
Looks good. Thanks for the tests! |
This continues the work from #2279, by allowing example badges to be specified using
namedParams. Using an object makes it possible for us to display these in form fields down the line. (#701)I've called this the "preferred" way, and labeled the other ways deprecated. I've also added some doc to the
examplesproperty in BaseService. Then I realized we had some doc in the tutorial, though I think it's fine to have a short version in the tutorial, and the gory detail in BaseService.I've also added a
patternkeyword, and madeurlPatternan alias.Closes #2050.