chore: use prettier plugin for jsdoc by shunkica · Pull Request #513 · xmldom/xmldom

shunkica · 2023-07-15T10:00:01Z

This PR includes a commit that adds the prettier-plugin-jsdoc dependency to the project.
The plugin will improve the readability and maintainability of our JSDoc comments and ensure a consistent style across the project.

socket-security · 2023-07-15T10:04:36Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@homer0/prettier-plugin-jsdoc	6.0.5	None	`+2`	1.86 MB	homer0

karfau

I'm not sure where those options should go (assume to the top level?), but I would prefer to enable the tsdoc and jsdocPreferCodeFences options.

karfau · 2023-07-15T19:12:44Z

@shunkica before you invest eny more time into this, I just had a closer look at the plugin from a dependency perspective, and there are some things that make me feel uncomfortable:

quite some number of reported issues (have you used it with a good/stable devx before?)
not much maintainer activity in the recent month at first glance
no compatibility with prettier v3 (I was hoping/planning to land chore(deps): update dependency prettier to v3 - autoclosed #504 soonish)
quite some issues detected on socket.dev (see https://socket.dev/npm/package/prettier-plugin-jsdoc/overview/0.4.2) ~~I think that's mostly because all it's dev dependencies are installed as dependencies, which is why it adds 36 dependencies to our already long list of devDependencies, which I would prefer to avoid.~~

That's not a "NO", but some things to consider.

I think I will file a PR over there that moves develeopment dependencies to the devDependencies part of package.json, and If this is landed and/or prettier v3 support is fixed (there is already a PR), I'have no issues with adding it.

Update I misread the dependencies over there, they are correct, it's just quite a heavy outdated tree... so it is just the prettier v3 support I'm worried about

shunkica · 2023-07-16T11:04:53Z

I have not used this plugin before, but since it is only a dev dependency, has over 40000 weekly downloads, and it did what it is supposed to, I have not given it that much thought.
If you have issues with that plugin, maybe this one would be better as it doesn't have that many dependencies, but it is a much (much) smaller project. @homer0/prettier-plugin-jsdoc

karfau · 2023-07-16T11:45:37Z

That looks much more reliable and alive, even though there are not as many downloads.
It is also not supporting prettier v3 yet, and v6 dropped node v14 support, but I think prettier v3 also requires node v16 so we can switch the eslint workflow to use node v16 along the way

homer0 · 2023-07-16T14:09:43Z

@karfau 👋 I just published the support for v3, and I'm planning to drop v16 before end of the year (since it's reaching EOL on October).

shunkica · 2023-07-16T14:35:21Z

@homer0 I have added @homer0/prettier-plugin-jsdoc v7.0.0 to the project, but when running format this error occurs a bunch of times.

Edit: nevermind, I see now in package.json that prettier v3 is requried for v7


TypeError: Cannot read properties of undefined (reading 'startsWith')
[error]     at printAngular (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:24775:22)
[error]     at printPathNoParens (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:29687:25)
[error]     at Object.genericPrint [as print] (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:29629:23)
[error]     at callPluginPrintFunction (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:8601:26)
[error]     at mainPrintInternal (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:8550:22)
[error]     at mainPrint (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:8537:18)
[error]     at printAstToDoc (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:8529:18)
[error]     at coreFormat (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:8837:20)
[error]     at formatWithCursor2 (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:9021:18)
[error]     at Object.formatWithCursor (/home/ivanclab/DevProjects/external/xmldom/node_modules/prettier/index.js:38183:12)

karfau · 2023-07-16T14:38:32Z

You might have to also bump prettier and eslint versions at the same time to make it work with v7.

Or try to make it work with v6 first

shunkica · 2023-07-16T14:41:45Z

You might have to also bump prettier and eslint versions at the same time to make it work with v7.

Or try to make it work with v6 first

Yes I figured that out. Do you want to use @homer0/prettier-plugin-jsdoc:6.0.5 and keep prettier version, or use 7.0.0 and bump prettier version?

karfau · 2023-07-16T14:45:48Z

Let's first make it work with the current prettier version, I can take care of the updates afterwards

This reverts commit 58acc86.

homer0 · 2023-07-16T15:37:28Z

I’ll take a look tonight (I’m GMT-3), but I tested it with the cli, vscode ext and my e2e suite and didn’t see anything like that. Most of the issues I had were about the code not changing, since Prettier made its api async and I wasn’t awaiting stuff. I’ll let you know how it goes.

…

On Sun, Jul 16, 2023 at 11:59 AM shunkica ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/dom-parser.js <#513 (comment)>: > + * an + * error + * that + * prevents + * further + * processing + * in + * most + * implementations. + * . + * @Property {boolean} [locator=true] Configures + * if + * the + * nodes + * created + * during + * parsing + * will + * have + * a + * `lineNumber` + * and + * a + * `columnNumber` + * + * attribute + * describing + * their + * location + * in + * the + * XML + * string. + * Default + * is + * true. + * Default + * is + * `true` + * @Property {(string) => string} [normalizeLineEndings] Used + * to + * replace + * line + * endings + * before + * parsing, + * defaults + * to + * `normalizeLineEndings` + * @Property {Object} [xmlns] The + * XML + * namespaces + * that + * should + * be + * assumed + * when + * parsing. + * The + * default + * namespace + * can + * be + * provided + * by + * the + * key + * that + * is + * the + * empty + * string. + * When + * the + * `mimeType` + * for + * HTML, + * XHTML + * or + * SVG + * are + * passed + * to + * `parseFromString`, + * the + * default + * namespace + * that + * will + * be + * used, + * will + * be + * overridden + * according + * to + * the + * specification. + * @see NormalizeLineEndings. */ @homer0 <https://github.com/homer0> any idea what happened here? — Reply to this email directly, view it on GitHub <#513 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVATPRMXJ7FFLUSJUXN6ALXQP6TRANCNFSM6AAAAAA2LGHO4U> . You are receiving this because you were mentioned.Message ID: ***@***.***>

shunkica · 2023-07-16T16:32:32Z

I’ll take a look tonight (I’m GMT-3), but I tested it with the cli, vscode ext and my e2e suite and didn’t see anything like that. Most of the issues I had were about the code not changing, since Prettier made its api async and I wasn’t awaiting stuff. I’ll let you know how it goes.

Thanks, please note though that this happened with version 6.0.5 I have not tested 7.0.0 since we are still on prettier v2.

homer0 · 2023-07-17T03:43:32Z

@shunkica 👋

I just took a look and this line was the issue:

/*
 * @property {typeof conventions.assign} [assign=Object.assign || conventions.assign]
 * The method to use instead of `Object.assign` (or if not available `conventions.assign`),
 * which is used to copy values from the options before they are used for parsing.
 */

Adding a parenthesis o r changing the default value of assign fixes the issue when formatting.

I know, it's not great that something like that would break the whole formatting, but that tends to happen when you start mixing a lot of TS functions with JSDoc, like that OR in the default value.

Some stuff may work, like the typeof on the type, and that's mostly magic from the IDE/Editor, but it can break other things, for example: You have the onError property on that same object, but if you check the inferred type, you'll see that the property doesn't have a name:

So the reference on onErrorStopParsing doesn't properly link there. That's because that TS syntax don't work on all context; you need to change the type to this in order to make it properly work:

/*
 * @property {(level:ErrorLevel, message:string, context: DOMHandler) => void} [onError]
 */

My recommendation, regardless of the formatting plugin you end up choosing, would be to always check with the IDE/Editor to make sure the inferred type make sense, and to be very careful with the default values, because I believe the editors will only recognize the brackets (making the param/property optional) and ignore the default value, so it will fallback to standard JSDoc, which doesn't accept very complex types.

I hope that helps :)

karfau · 2023-07-17T07:14:44Z

Thx a lot, if adding the default value this way is not reliable we have to do it differently.
@shunkica if you are not able to get it working let me know and I will have a look

shunkica · 2023-07-17T10:27:02Z

There is also some kind of conflict between eslint and prettier now, because they produce different versions of the jsdoc. After running prettier format, eslint complains about a bunch of stuff (spaces and such)

karfau · 2023-07-17T11:48:23Z

Since it is the eslint prettier/prettier rule that is complaining, I will take care of it by separating eslint and prettier.
From my experience in other projects this is what most people do now anyways, also for eslint performance problems when using it to run prettier.

shunkica · 2023-07-17T13:08:16Z

It turns out the column option was the one causing a bit of a hiccup. I've decided to turn it off for now, as it wasn't really meshing with the overall aesthetic. Hope that's alright!

karfau

I didn't went through all of the changes, sicne there are already quite some comments/questions.

Feel free to ignore the "content corrections" if you think they are in the way of getting the formatting right, we can also apply them later.

Thx for investing the time, I hope we can find a way to resolve all the open questions and can come up with a config that provides consistency without getting in our way.

This reverts commit 3422cf2.

Co-authored-by: Christian Bewernitz <[email protected]>

shunkica · 2023-07-18T08:17:02Z

@karfau is there a reason yml files are used for config? Would you mind a switch to json? ( for prettier and eslint)

karfau · 2023-07-18T14:48:12Z

@karfau is there a reason yml files are used for config? Would you mind a switch to json? ( for prettier and eslint)

I'm not aware of any specific reason other than it being the default documented format?

shunkica · 2023-07-18T17:23:44Z

@karfau is there a reason yml files are used for config? Would you mind a switch to json? ( for prettier and eslint)

I'm not aware of any specific reason other than it being the default documented format?

Its one of the documented formats. I wouldn't say it is default.
I just find JSON easier to work with than some indentation based markup.
In any case It's not a big issue for me.

karfau

@shunkica I'm fine with the current state.

I configured the jsdocPrintWidth to 95, which is of course an arbitrary number, but I tried different values and this one looked best for the existing content as far as I could judge it.
(I did invest some time into formatting before this new mechanism, so even if it was not totally consistent it was also not without a any consideration.)

Adn I think I would prefer changing the config format in the next PR if you don't mind.

Let me know if there is anything you want ot add to this PR.

shunkica · 2023-07-19T02:03:58Z

@shunkica I'm fine with the current state.

I configured the jsdocPrintWidth to 95, which is of course an arbitrary number, but I tried different values and this one looked best for the existing content as far as I could judge it. (I did invest some time into formatting before this new mechanism, so even if it was not totally consistent it was also not without a any consideration.)

Adn I think I would prefer changing the config format in the next PR if you don't mind.

Let me know if there is anything you want ot add to this PR.

I stopped working yesterday because the IDE was was hammering my cpu to 250% yesterday and became unusable. I suspected the culprit was eslint/prettier but couldn't be bothered to investigate further at the time.

homer0 · 2023-07-19T02:35:18Z

@shunkica I'm fine with the current state.

I configured the jsdocPrintWidth to 95, which is of course an arbitrary number, but I tried different values and this one looked best for the existing content as far as I could judge it. (I did invest some time into formatting before this new mechanism, so even if it was not totally consistent it was also not without a any consideration.)

Adn I think I would prefer changing the config format in the next PR if you don't mind.

Let me know if there is anything you want ot add to this PR.

I stopped working yesterday because the IDE was was hammering my cpu to 250% yesterday and became unusable. I suspected the culprit was eslint/prettier but couldn't be bothered to investigate further at the time.

If you are using vscode: whenever you change Prettier/ESLint configs, or install/update dependencies, do a "reload window"; Linting tooling usually becomes buggy because it already had stuff in memory.

And just in case, no idea if you are doing it but, don't try to run eslint --fix with Prettier. ESLint is REALLY slow compared to Prettier, added to how fast you can be coding… it creates some fun race conditions :P.

shunkica · 2023-07-19T02:39:07Z

If you are using vscode: whenever you change Prettier/ESLint configs, or install/update dependencies, do a "reload window"; Linting tooling usually becomes buggy because it already had stuff in memory.

And just in case, no idea if you are doing it but, don't try to run eslint --fix with Prettier. ESLint is REALLY slow compared to Prettier, added to how fast you can be coding… it creates some fun race conditions :P.

I am actually using WebStorm, but I didn't have time to figure out what was going on.

karfau · 2023-07-19T08:17:34Z

@shunkica let me know if you are still observing this slowness after a fresh computer/IDE start.

If yes, I will wait for you to figure out what is going on.

From the npm scripts we don't do eslint --fix, but I guess it is possible to invoke it from the IDE.

I think it is time to apply prettier to staged files in a pre-commit hook asap, to avoid running into formatting issues in the github actions. But this should be a separate PR.

Thx a lot @homer0 for all the support and for building and maintaining the plugin.

shunkica · 2023-07-19T09:14:55Z

I can not reproduce the problems I had yesterday.

ivanclab added 2 commits July 15, 2023 11:54

add prettier-plugin-jsdoc

58acc86

format

464751d

shunkica added dependencies Pull requests that update a dependency file code style labels Jul 15, 2023

karfau reviewed Jul 15, 2023

View reviewed changes

ivanclab added 3 commits July 16, 2023 16:50

Revert "add prettier-plugin-jsdoc"

8c2dc14

This reverts commit 58acc86.

add dev dep @homer0/[email protected]

f560421

format

696a52c

shunkica commented Jul 16, 2023

View reviewed changes

Comment thread lib/dom-parser.js

karfau changed the title ~~Add prettier-plugin-jsdoc~~ chore: add prettier-plugin-jsdoc Jul 17, 2023

ivanclab added 2 commits July 17, 2023 14:59

disable jsdoc columns

438aa17

format

3422cf2

karfau reviewed Jul 18, 2023

View reviewed changes

Revert "format"

d78b282

This reverts commit 3422cf2.

Update lib/sax.js

c5ded20

Co-authored-by: Christian Bewernitz <[email protected]>

ivanclab and others added 9 commits July 18, 2023 19:28

move deprecated to top of jsdocTagsOrder

fac6a71

disallow jsdoc access tag

673d431

docs(ParseError): proper syntax for optional locator

33f5840

docs: reduce jsdocPrintWidth and restore links

1a150e1

docs: correct syntax for error types

99b7e53

docs: restore more links

465adb8

docs: restore more links/correct syntax

a178fb0

docs: restore more links/correct syntax

6f6eff7

style: apply prettier

a563da9

karfau approved these changes Jul 18, 2023

View reviewed changes

karfau changed the title ~~chore: add prettier-plugin-jsdoc~~ chore: use prettier plugin for jsdoc Jul 19, 2023

karfau merged commit a75f3d3 into xmldom:master Jul 19, 2023

shunkica deleted the prettier-jsdoc branch July 20, 2023 16:46

noushiyo mentioned this pull request Sep 26, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.8.10 to 0.9.1 noushiyo/18_1_2.0#2

Open

This was referenced Sep 26, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.8.7 to 0.9.1 FitzwilliamMuseum/fitz-main-website#711

Closed

[Snyk] Upgrade @xmldom/xmldom from 0.8.2 to 0.9.1 FitzwilliamMuseum/fitz-collection-online#247

Closed

idleberg mentioned this pull request Sep 26, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.8.10 to 0.9.1 idleberg/gulp-xml-validator#6

Closed

Soontao mentioned this pull request Sep 26, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.8.10 to 0.9.1 Soontao/RSSHub#54

Open

deepakprabhakara mentioned this pull request Sep 27, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.8.10 to 0.9.2 ory/saml20#691

Closed

ITSecOps-404 mentioned this pull request Oct 5, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.8.10 to 0.9.2 ITSecOps-404/app#1

Open

1AhmedYasser mentioned this pull request Oct 27, 2024

[Snyk] Upgrade @xmldom/xmldom from 0.7.13 to 0.9.3 1AhmedYasser/Chat-Widget#1

Open

Uh oh!

Conversation

shunkica commented Jul 15, 2023

Uh oh!

socket-security Bot commented Jul 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karfau left a comment

Choose a reason for hiding this comment

Uh oh!

karfau commented Jul 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shunkica commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karfau commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

homer0 commented Jul 16, 2023

Uh oh!

shunkica commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karfau commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shunkica commented Jul 16, 2023

Uh oh!

karfau commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

homer0 commented Jul 16, 2023 via email

Uh oh!

shunkica commented Jul 16, 2023

Uh oh!

homer0 commented Jul 17, 2023

Uh oh!

karfau commented Jul 17, 2023

Uh oh!

shunkica commented Jul 17, 2023

Uh oh!

karfau commented Jul 17, 2023

Uh oh!

shunkica commented Jul 17, 2023

Uh oh!

karfau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shunkica commented Jul 18, 2023

Uh oh!

karfau commented Jul 18, 2023

Uh oh!

shunkica commented Jul 18, 2023

Uh oh!

karfau left a comment

Choose a reason for hiding this comment

Uh oh!

shunkica commented Jul 19, 2023

Uh oh!

homer0 commented Jul 19, 2023

Uh oh!

shunkica commented Jul 19, 2023

Uh oh!

karfau commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shunkica commented Jul 19, 2023

Uh oh!

socket-security Bot commented Jul 15, 2023 •

edited

Loading

karfau commented Jul 15, 2023 •

edited

Loading

shunkica commented Jul 16, 2023 •

edited

Loading

karfau commented Jul 16, 2023 •

edited

Loading

shunkica commented Jul 16, 2023 •

edited

Loading

karfau commented Jul 16, 2023 •

edited

Loading

karfau commented Jul 16, 2023 •

edited

Loading

karfau commented Jul 19, 2023 •

edited

Loading