Skip to content
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

fix: ensure Electron.Utility property exists #247

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

devm33
Copy link
Contributor

@devm33 devm33 commented Oct 19, 2023

Added in #246, the Utility namespace currently has no concrete values in it on electron/electron@main. This change ensures Electron.Utility is available as a property even when the namespace has no values.

Takes advantage of declaration merging: https://www.typescriptlang.org/docs/handbook/declaration-merging.html

Other approaches considered:

  • Separating out the values from each namespace into interfaces and setting properties for each interface.
    • Decided this was too invasive for questionable value
  • Adding a dummy value to the Utility namespace to ensure it's never empty.
    • Decided too much of a hack and dummy value would be visible to end users

Tested with node spec/ts-smoke/runner.js on electron/electron@main

Should unblock electron/electron#40264 to ultimately be used (and made not empty) in electron/electron#40017

@dsanders11
Copy link
Member

Should that line be guarded with a check that UtilityNamespace.length > 2? Doing a quick test I'm not sure declaration merging works in this situation since one is a namespace and the other is a value. It doesn't error if the namespace is empty, but if I put anything in the namespace, I see error TS2300: Duplicate identifier 'Utility'.

@devm33
Copy link
Contributor Author

devm33 commented Oct 20, 2023

Should that line be guarded with a check that UtilityNamespace.length > 2? Doing a quick test I'm not sure declaration merging works in this situation since one is a namespace and the other is a value. It doesn't error if the namespace is empty, but if I put anything in the namespace, I see error TS2300: Duplicate identifier 'Utility'.

Ah thanks for checking that. I see the declaration merging only works when the namespace doesn't have any values because otherwise it's trying to merge two variables.

Added a check to only set the property if the Utility namespace doesn't have any values: 3091893

@devm33
Copy link
Contributor Author

devm33 commented Oct 25, 2023

@MarshallOfSound friendly bump when you're able to review 🙏

@dsanders11
Copy link
Member

@devm33, we're good to go here, just a merge conflict due to #249 being merged. Once you resolve that I'll merge this. 👍

Added in electron#246, the Utility namespace currently has no concrete values in
it on electron/electron@main. This change ensures Electron.Utility is
available as a property even when the namespace has no values.
If the Utility namespace has concrete values and the const Utility
property is set it causes `error TS2300: Duplicate identifier 'Utility'.`
This tracks whether any values are added to the Utility namespace and
only sets the property workaround if there are none.
@devm33 devm33 force-pushed the devm33/ensure-namespace-properties branch from 3091893 to 89617c3 Compare October 25, 2023 20:53
@devm33
Copy link
Contributor Author

devm33 commented Oct 25, 2023

@devm33, we're good to go here, just a merge conflict due to #249 being merged. Once you resolve that I'll merge this. 👍

Awesome, thanks @dsanders11! Conflict resolved.

Once merged and published, I'll update electron/electron#40264

@dsanders11 dsanders11 merged commit 8269bce into electron:main Oct 25, 2023
3 checks passed
@continuous-auth
Copy link

🎉 This PR is included in version 8.15.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@devm33 devm33 deleted the devm33/ensure-namespace-properties branch October 26, 2023 02:22
devm33 added a commit to devm33/electron that referenced this pull request Oct 26, 2023
Upgrades docs-parser and typescript-definitions to add new
'electron/utility' namespace added in:
- electron/docs-parser#95
- electron/typescript-definitions#246
- electron/typescript-definitions#247
jkleinsc pushed a commit to electron/electron that referenced this pull request Oct 31, 2023
…ons (#40264)

* feat: add utility process typescript namespace

Upgrades docs-parser and typescript-definitions to add new
'electron/utility' namespace added in:
- electron/docs-parser#95
- electron/typescript-definitions#246
- electron/typescript-definitions#247

* build: update yarn.lock

---------

Co-authored-by: David Sanders <[email protected]>
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
…ons (electron#40264)

* feat: add utility process typescript namespace

Upgrades docs-parser and typescript-definitions to add new
'electron/utility' namespace added in:
- electron/docs-parser#95
- electron/typescript-definitions#246
- electron/typescript-definitions#247

* build: update yarn.lock

---------

Co-authored-by: David Sanders <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants