Add support for logger labels #778
Conversation
| server: 133, | ||
| client: 'rgb(127,42,127)' | ||
| } | ||
| }); |
There was a problem hiding this comment.
Ooh, this is nicer output even without a label!
There was a problem hiding this comment.
yeah that hack was pretty hacky...
| // the code again from the ast) or change the module tagger to take an | ||
| // options object, rather than a string. | ||
| const arg = node.arguments[0]; | ||
| options.label = arg.properties[0].value.value; |
There was a problem hiding this comment.
loggerSpec actually expects the label to be passed in, and it incorporates it into the name.
So I think something like this would work:
const label = arg.properties[0].value.value;
const options = JSON.parse(loggerSpec({ filePath, trim, prefix, label }));Though we might be able to more confidently pull out the value associated with a label key (rather than the value associated with the first key) with something like:
const {label} = arg.properties.reduce((o,p) => (o[p.key.value] = p.value.value, o), {});Not familiar with babel plugins, so don't know if that's the right structure for the entries in arg.properties.
There was a problem hiding this comment.
I wish it was that nice; optString also requires the parens from the function invocation; e.g.
const label = arg.properties[0].value.value;
const options = JSON.parse(loggerSpec({ filePath, trim, prefix, optString: `{( label: ${label })` }));
I deferred implementation, though, since I'm unclear what the better change is: should I change the loggerSpec api to take an options object, or should I use babel-generator to turn the ObjectExpression I have in hand from the AST into a string, put some parens around it and pass that to loggerSpec. Both have downsides: the first introduces a risk of regression on all the other module taggers, and introduces a breaking change; but the second introduces a dependency I shouldn't really need (and using a code generator in a code generator feels... dicey).
There was a problem hiding this comment.
the first introduces a risk of regression on all the other module taggers
We control them all, though, right? So it's just a matter of updating our modules to conform to a new interface? Or am I misunderstanding this option?
There was a problem hiding this comment.
hypothetically, someone could be using react-server-module-tagger directly
| properties.push(t.objectProperty(t.identifier(key), literal)); | ||
| }); | ||
| return t.objectExpression(properties); | ||
| } |
There was a problem hiding this comment.
This is awesome. 🤘
Thanks for tackling this @doug-wade!
Fixes #738 by adding support for labels