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

Remove Node.js and DOM type references from core-common #7720

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

paulius-valiunas
Copy link
Contributor

No description provided.

@paulius-valiunas paulius-valiunas requested review from a team as code owners February 18, 2025 11:58
Copy link
Contributor

mergify bot commented Feb 18, 2025

This pull request is now in conflicts. Could you fix it @paulius-valiunas? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

@paulius-valiunas
Copy link
Contributor Author

@mergify queue

Copy link
Contributor

mergify bot commented Feb 19, 2025

queue

🟠 Waiting for conditions to match

  • -conflict [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = iTwin.js
        • check-skipped = iTwin.js
        • check-success = iTwin.js
      • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • any of: [🛡 GitHub branch protection]
        • check-success = license/cla
        • check-neutral = license/cla
        • check-skipped = license/cla
      • any of: [🛡 GitHub branch protection]
        • check-success = extract-api
        • check-neutral = extract-api
        • check-skipped = extract-api
      • any of: [🛡 GitHub branch protection]
        • check-success = iTwin.js Integration - GitHub
        • check-neutral = iTwin.js Integration - GitHub
        • check-skipped = iTwin.js Integration - GitHub
      • any of: [🛡 GitHub branch protection]
        • check-success = iTwin.js Docs - YAML
        • check-neutral = iTwin.js Docs - YAML
        • check-skipped = iTwin.js Docs - YAML
  • -closed [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@paulius-valiunas paulius-valiunas linked an issue Feb 19, 2025 that may be closed by this pull request
Copy link
Contributor

mergify bot commented Feb 19, 2025

This pull request is now in conflicts. Could you fix it @paulius-valiunas? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

@paulius-valiunas
Copy link
Contributor Author

paulius-valiunas commented Feb 20, 2025

@tcobbs-bentley could you help with the failing Android build? I don't have the environment for that and I'm not sure what could be causing it... maybe just setting breakpoints at the lines I changed would show that some of the expected globals are undefined?

@tcobbs-bentley
Copy link
Member

@tcobbs-bentley could you help with the failing Android build? I don't have the environment for that and I'm not sure what could be causing it... maybe just setting breakpoints at the lines I changed would show that some of the expected globals are undefined?

I will investigate. If it's something trivial, I'll probably push changes to this PR.

@tcobbs-bentley
Copy link
Member

When running in Android I get a run-time exception:

Uncaught (in promise) ReferenceError: global is not defined
    at get instance (index-Dm76TGMn.js:109130:25)
    at RpcManager.initializeInterface (index-Dm76TGMn.js:109400:17)
    at new MobileIpcTransport (index-Dm76TGMn.js:246995:16)
    at new MobileRpcProtocol (index-Dm76TGMn.js:247122:17)
    at <instance_members_initializer> (index-Dm76TGMn.js:247403:18)
    at new config (index-Dm76TGMn.js:247401:20)
    at RpcConfiguration.obtain (index-Dm76TGMn.js:108944:56)
    at MobileRpcManager.performInitialization (index-Dm76TGMn.js:247408:40)
    at MobileRpcManager.initializeClient (index-Dm76TGMn.js:247414:29)
    at MobileApp.startup (index-Dm76TGMn.js:247507:24)

I'll see if adding a declare const like for self and window fixes the problem.

@@ -44,7 +47,7 @@ export class RpcRegistry {

public static get instance() {
if (!RpcRegistry._instance) {
const globalObj: any = typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {};
const globalObj: any = global ?? self ?? window ?? {};
Copy link
Member

Choose a reason for hiding this comment

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

This is the line that is throwing an exception in Android. Updating it to be the following fixes the problem in Android, but I believe that you need to revert all of your changes that abandoned the old typeof checks, because the new code is fundamentally different (and definitely broken for global in Android, and probably broken for self and window):

      const globalObj: any = (typeof global !== "undefined" ? global : undefined) ?? self ?? window ?? {};

What I mean by that is that both self and window are defined in Android, but if they weren't, then my updated line above would not work, while the original code would. Adding the following doesn't do any good, because declare const just lets the TypeScript compiler know that a variable is supposed to exist; it does not actually create the variable:

declare const global: any;

Because of this, you may need to remove the declare const statements that you added (in addition to switching back to the old typeof x !== "undefined" checks with ternary operators).

Copy link
Member

@tcobbs-bentley tcobbs-bentley Feb 20, 2025

Choose a reason for hiding this comment

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

If you really don't like the old code, you could instead do something like this:

  private static get anyGlobal(): any {
    return typeof global !== "undefined" ? global : undefined;
  }
...
      const globalObj: any = RpcRegistry.anyGlobal ?? self ?? window ?? {};

Do the same for anySelf and anyWindow.

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.

ResponseLike in RpcRequest.ts uses DOM types
5 participants