feat(rome_js_analyze): handle serde errors inside RuleContext
#3352
Conversation
ematipico
changed the title
RuleContext
ematipico
changed the title
RuleContext RuleContext
ematipico
force-pushed
the
feature/rule-with-self
branch
from
bb271aa to
2f8980e
Compare
✅ Deploy Preview for rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Comparing feat(rome_js_analyze): handle serde errors inside
1 page tested
|
| Chrome Desktop | iPhone, 4G LTE | Motorola Moto G Power, 3G connection |
|---|---|---|
 |
 |
 |
Most significant changes
 |
Value | Budget |
|---|---|---|
| Total JavaScript Size in Bytes Chrome Desktop |
1.24 MB  from 86.8 KB |
 |
| Total JavaScript Size in Bytes iPhone, 4G LTE |
1.24 MB from 86.8 KB |
|
| Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection |
1.24 MB from 86.8 KB |
|
| Number of Requests Chrome Desktop |
39  from 5 |
|
| Number of Requests iPhone, 4G LTE |
39 from 5 |
|
4 other significant changes: Number of Requests on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection
Calibre: Site dashboard | View this PR | Edit settings | View documentation
crates/rome_analyze/src/options.rs
Outdated
| @@ -53,3 +55,51 @@ pub struct AnalyzerOptions { | |||
| /// A data structured derived from the [`rome.json`] file | |||
| pub configuration: AnalyzerConfiguration, | |||
| } | |||
|
|
|||
| #[derive(Debug, Diagnostic)] | |||
| #[diagnostic(category = "internalError/io", tags(INTERNAL))] | |||
There was a problem hiding this comment.
The exact meaning of the INTERNAL tag is a bit vague, but in general I think it should be used for errors that are not caused by a direct action or input from the user. In this case, since this error results from deserializing a piece of user-provided input (the rome.json configuration file) so it doesn't really qualify as "internal"
There was a problem hiding this comment.
I removed it. The only reason I had it there, was mostly because I copied the code the the examples/ folder. Thank you for catching it.
| Self { | ||
| message: message.clone(), | ||
| description: message, | ||
| path: Resource::Memory, |
There was a problem hiding this comment.
Would it be possible to inject the path to the rome.json file here ? It would be more accurate but might incur a complex refactor though ...
There was a problem hiding this comment.
Unfortunately we don't have that information yet... but we might able to get something out, once we merge #3381
At the moment the analyzer doesn't have any awareness at all, we need to construct AnalyzerOptions to hold all the information we need, including the path to the configuration. We don't have this info yet (neither the Workspace has it, at the time of writing)
Summary
Closes #3346
This PR does the following:
RawValueand useValuefor the time being;serdehas a nasty issue whereuntaggedmacro doesn't work withRawValue. Fixing the issue in user land is not an easy task. So as for now, myself and @leops decided to useValuefor the time being. This allows us to correctly parseoptions, but we would need to parse it again in order to type it correctly. This is temporary measure while we will find a good way to re-introduceRawValue. Link to the bug Internal buffering disrupts format-specific deserialization features serde-rs/serde#1183RuleContextDiagnosticwhich uses the new diagnostics, which emits errors in case deserialisation fails or the rule can't find the services needed fromServiceBag. Unfortunately I can't test the diagnostic yet because there's still some missing logic yet (creation ofAnalyzerOptionsfrom theWorkspace). A test will be created once all the pieces are in place.RulecalledOptions, which is used to dynamically type the options, which are retrieved using the methodctx.options(). Because of that, all the new rules had to be updated to take this new type into account.Test Plan
The new logic is still not triggered, so the CI should just pass