Bump eslint-config-pixiebrix from 0.19.1 to 0.20.0 #4721
Conversation
![dependabot[bot]](https://avatars.githubusercontent.com/in/29110?s=60&v=4){kind=small}
Bumps [eslint-config-pixiebrix](https://github.com/pixiebrix/eslint-config-pixiebrix) from 0.19.1 to 0.20.0. - [Release notes](https://github.com/pixiebrix/eslint-config-pixiebrix/releases) - [Commits](pixiebrix/eslint-config-pixiebrix@v0.19.1...v0.20.0) --- updated-dependencies: - dependency-name: eslint-config-pixiebrix dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
dependabot
bot
added
dependencies
Codecov Report
@@ Coverage Diff @@
## main #4721 +/- ##
==========================================
- Coverage 53.00% 52.97% -0.03%
==========================================
Files 922 922
Lines 27306 27288 -18
Branches 5272 5268 -4
==========================================
- Hits 14474 14457 -17
+ Misses 12832 12831 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| if (Object.prototype.hasOwnProperty.call(current, serviceAuthId)) { | ||
| // eslint-disable-next-line security/detect-object-injection -- just checked with `hasOwnProperty` | ||
| if (Object.hasOwn(current, serviceAuthId)) { | ||
| // eslint-disable-next-line security/detect-object-injection -- just checked with `hasOwn` |
There was a problem hiding this comment.
| @@ -369,7 +369,7 @@ export async function launchOAuth2Flow( | |||
| throw new BusinessError("authorizeUrl is required for oauth2"); | |||
| } | |||
|
|
|||
| const isImplicitFlow = typeof rawTokenUrl === "undefined"; | |||
| const isImplicitFlow = rawTokenUrl === undefined; | |||
There was a problem hiding this comment.
| ]); | ||
| const prop = freshIdentifier( | ||
| "property" as SafeString, | ||
| Object.keys(draft) |
There was a problem hiding this comment.
|
|
||
| if (typeof OmitFieldWidget === "undefined" || OmitFieldWidget == null) { |
There was a problem hiding this comment.
Redundant check with == null
| for (const [name, widget] of Object.entries(defaultWidgets)) { | ||
| if (!widget) { | ||
| throw new Error( | ||
| `Error registering default widgets. ${name} is undefined. Is there a circular dependency?` |
There was a problem hiding this comment.
This loop checks all the widgets, not just 2. This is the only runtime change and it has been verified in the page editor
| <Col xs={12} sm={6} xl={4}> | ||
| <Card key={serviceId} className={styles.serviceCard}> | ||
| <Col xs={12} sm={6} xl={4} key={serviceId}> | ||
| <Card className={styles.serviceCard}> |
There was a problem hiding this comment.
There was a problem hiding this comment.
I understand the lint rule change and why the key needs to be on the top level of the optional child here, but the key is now on the entire parent Col rather than just the Card. There are other components in the children of this Col. Does this change affect how those components re-render when/if the serviceId changes? Was this also thoroughly tested?
There was a problem hiding this comment.
There are other components in the children of this
Col
Col has only one child: Card
What's inside Col doesn't matter anyway as it's tracked as a whole block. As long as the direct child in a loop has the key, there's no problem.
If we used React.StrictMode we'd even get a runtime warning: https://codepen.io/fregante/pen/bGKjqJe?editors=0011
Does this change affect how those components re-render when/if the serviceId changes?
If it does, it's not in a way that matters, it just gives the correct diffing signals at a low level.
There was a problem hiding this comment.
Oh sorry, didn't see that the card wraps everything, never mind 👍
| @@ -30,7 +30,7 @@ | |||
| // Help Madge resolve this dependency | |||
| "marked": ["node_modules/marked/lib/marked.esm.js"] | |||
| }, | |||
| "lib": ["ES2021", "DOM", "DOM.Iterable"], | |||
| "lib": ["ES2022", "DOM", "DOM.Iterable"], | |||
There was a problem hiding this comment.
I verified that our current Chrome version (in the manifest) supports 100% of ES2022 already, so we're covered
| @@ -89,7 +89,6 @@ export async function waitForTargetByUrl(url: string): Promise<Target> { | |||
|
|
|||
| /** | |||
| * Run a brick in the window that opened the source window | |||
| * @param request | |||
fregante
deleted the
dependabot/npm_and_yarn/eslint-config-pixiebrix-0.20.0
branch
| <Col xs={12} sm={6} xl={4}> | ||
| <Card key={serviceId} className={styles.serviceCard}> | ||
| <Col xs={12} sm={6} xl={4} key={serviceId}> | ||
| <Card className={styles.serviceCard}> |
There was a problem hiding this comment.
I understand the lint rule change and why the key needs to be on the top level of the optional child here, but the key is now on the entire parent Col rather than just the Card. There are other components in the children of this Col. Does this change affect how those components re-render when/if the serviceId changes? Was this also thoroughly tested?
| @@ -427,7 +427,6 @@ export function readerTypeHack(reader: ReaderConfig): SingleLayerReaderConfig { | |||
| * Normalize the pipeline prop name and assign instance ids for tracing. | |||
| * @param config the extension configuration | |||
| * @param pipelineProp the name of the pipeline prop, currently either "action" or "body" | |||
| * @param defaults | |||
There was a problem hiding this comment.
As I read more of these, I'm not sure that I agree we should be removing all the name-only @param s like this. Is there a reason to remove them? It still gives a tiny bit of a visual hint for the function inputs when looking at a function definition. I'm not sure I see the harm in keeping them?
There was a problem hiding this comment.
I'm not sure I see the harm in keeping them?
I see several issues:
- It doesn't actually add any information whatsoever
- Having unnecessary lines slows down the reading of code, because my eyes are now on
@param valuewhile they could just be on the actualvalue: Valueline and get Type information instead - It can get out of sync. Try adding
@param misspelledVaranywhere and our tooling won't complain. Now the type definition mentions some non-existentmisspelledVarparameter.
As for this specific line, it's the only place where other parameters have been described and this one wasn't. I suppose an argument could be made to leave this one for "consistency among the other parameters." 👍
All the other cases however mention one @param name and then nothing else:
/**
* Return utility classes from the value
* @param value
*/
export function parseValue(value: Value): {The above doesn't add value over:
/**
* Return utility classes from the value
*/
export function parseValue(value: Value): {visual hint for the function inputs when looking at a function definition
The function definition already has the actual parameter a few pixels away, it doesn't need another hint.
Is there a reason to remove them?
Would you keep this spacing?
/**
* Return utility classes from the value
*/
export function parseValue(value: Value): {You might argue that there's no reason to remove the spacing, it does make the code more pleasant, but we live in a society 😜
There was a problem hiding this comment.
It doesn't actually add any information whatsoever
Sure it does, it tells you the names of the function input params without having to read any of the function code itself.
Having unnecessary lines slows down the reading of code, because my eyes are now on @param value while they could just be on the actual value: Value line and get Type information instead
Not every function just has the params right there on the next line. Some of the examples in this PR have type hints and more complicated inputs, so it can be helpful to just see the params listed at the top there. Also, I disagree that having more lines of comments slows down reading of code. This is a comment, it's not even code. And it's only a few characters of comments.
It can get out of sync. Try adding @param misspelledVar anywhere and our tooling won't complain. Now the type definition mentions some non-existent misspelledVar parameter.
No, it can't. We have a lint rule for that already.
There was a problem hiding this comment.
It doesn't actually add any information whatsoever
Sure it does, it tells you the names of the function input params without having to read any of the function code itself.
But I can read the function code without having to read any comments. It goes both ways, except one line is required and the JSDoc comment is not.
Plus no information is added, but just duplicated.
We don't need to repeat the same piece of information in multiple parts of the same function. By your logic we should also describe the same variable every time it's used so we don't have to move our eyes higher: it would be useful but it's not practical and it has drawbacks.
Some of the examples in this PR have type hints and more complicated inputs
I don't see any such examples that used @param something without a description (in which case they'd be fine for me).
I disagree that having more lines of comments slows down reading of code.
I suppose I can add unnecessary comments everywhere since they don't slow down the reading of code
// The session ID
export const sessionId = uuidv4();
// The timestamp of the session
export const sessionTimestamp = new Date();
// The ID of the navigation
export let navigationId = uuidv4();
// The navigation timestamp. It stamps the time of the navigation with a timestamp.
export let navigationTimestamp = new Date();There are useful comments and there are useless comments.
It can get out of sync. Try adding @param misspelledVar anywhere and our tooling won't complain. Now the type definition mentions some non-existent misspelledVar parameter.
No, it can't. We have a lint rule for that already.
I'm not sure why you'd claim that instead of trying my suggestion. The CI on d9b521f2 is green. We don't have any strict JSDoc lint rules.
Bumps eslint-config-pixiebrix from 0.19.1 to 0.20.0.
Release notes
Sourced from eslint-config-pixiebrix's releases.
Commits
b4796f00.20.05443d98Cleanupb91598dBump actions/* from v2 to v3 (#10)18337abBump eslint-plugin-unicorn from 44.0.2 to 45.0.0 (#13)80a8751Bump@typescript-eslint/eslint-pluginfrom 5.42.1 to 5.44.0 (#14)1e8d310Bump typescript from 4.8.4 to 4.9.3 (#15)86ac416Bump eslint-config-xo-typescript from 0.54.1 to 0.55.0 (#16)1a9439aEnable dependabotDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)