Skip to content

Add more comments #1993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 81 commits into from
Jun 3, 2025
Merged

Add more comments #1993

merged 81 commits into from
Jun 3, 2025

Conversation

Bashamega
Copy link
Contributor

related to #1940
Hello:)
In my last PR I have used MDN to generate comments for interface declarations, in this pr I add it for the properties.

Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 23, 2025

Not ready for review
Ready for review

Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm yet to review this, but please do not convert to sync APIs.

@Bashamega
Copy link
Contributor Author

I'm yet to review this, but please do not convert to sync APIs.

Sure

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 23, 2025

Hello @saschanaz
I have switched the old functions to async, but the new function is still sync because the emitter is not async. I tried switching it to async, but it broke.

@Bashamega
Copy link
Contributor Author

I have updated the context to not call the API in the emitter, but that cause the test to fail, I will fix it later

@Bashamega
Copy link
Contributor Author

Done @saschanaz

@saschanaz
Copy link
Contributor

LGTM 🎉

Copy link
Contributor

github-actions bot commented Jun 3, 2025

There was an issue merging, maybe try again saschanaz. Details

@Bashamega
Copy link
Contributor Author

Thank you very much @saschanaz

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this, there's nothing else to comment on, right? This is as large as we're going to get?

Each one of these makes parsing the DOM types slower, so I'm hoping that we stop at some point 😅

@saschanaz
Copy link
Contributor

Technically we still have events, but adding comments there would be a bit awkward as that would only work for onevent attributes and not for addEventListener calls. Probably fair to just skip that.

@saschanaz
Copy link
Contributor

LGTM

@github-actions github-actions bot merged commit 5bd485e into microsoft:main Jun 3, 2025
6 checks passed
Copy link
Contributor

github-actions bot commented Jun 3, 2025

Merging because @saschanaz is a code-owner of all the changes - thanks!

@Bashamega
Copy link
Contributor Author

Bashamega commented Jun 4, 2025

Hello @jakebailey
First of all, thank you very much for merging this pr.
Secondly, I think we can improve the compiler's speed by caching comments in a JSON file, which will serve as the comments reference. The GitHub action will be responsible for updating it. If this sound good to you, I can draft a pr for it and compare the speed.
Also, there are still some missing comments.

@jakebailey
Copy link
Member

I don't know what you mean, sorry. The comments have to be in a declaration file. Moving them elsewhere won't speed things up (and will make the compiler/editor stuff challenging).

@Bashamega
Copy link
Contributor Author

I don't know what you mean, sorry. The comments have to be in a declaration file. Moving them elsewhere won't speed things up (and will make the compiler/editor stuff challenging).

I mean instead of fetching the data each time you run the generate command, we can put the comments in a json file and the builder will use that json file instead of fetching the summaries every time

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Jun 4, 2025

I am pretty sure Jake meant the usage (in the editor and compile of user code) of the generated files will slow down the typescript compiler as it has to parse more data.
Build/generation time of these files is uninteresting.
tsdoc belongs into the .d.ts file and not in a (faster?) parallel world which needs to be invented and maintained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants