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

[UI5WC 1.24.x / UI5WC4R 1.29.x Upgrade] Cannot read properties of null (reading '_namespaceURI') #10181

Closed
1 task done
patrickwang10 opened this issue Nov 14, 2024 · 8 comments
Assignees
Labels
author action bug This issue is a bug in the code

Comments

@patrickwang10
Copy link

Bug Description

Issue found in test failures from upgrading SAC's version of UI5WC from 1.23.3 => 1.24.13
Upgrade change: https://github.wdf.sap.corp/orca/uqm/pull/39980
Logs:
testLog

Reproduction:
Run VarianceSection.test.tsx test in UQM repo with the above change that consumes UI5WC 1.24.13 (npm run jest-debug packages/sap/viz-design-panel/test/ui/feed/VarianceSection)

Investigation:
NPE exception thrown at file:///C:/SAPDevelop/uqm/node_modules/jsdom/lib/jsdom/living/custom-elements/CustomElementRegistry-impl.js -> "candidate._namespaceURI"
NPE is due to Window._document object being undefined.
Window._document object is undefined because it is cleared in the jsdom APIs by the jest framework during clean up steps.
The jest clean up steps are executed at the right time, but the call to CustomElementRegistry is done way too late (after all the test execution steps are completed).
The problem call originates from file:///C:/SAPDevelop/uqm/node_modules/@ui5/webcomponents-base/dist/UI5Element.js -> define() -> customElements.define(tag, this); (further originating from TimePicker)

UI5Element define

The define() call in UI5Element for TimePicker is first called before any tests are executed, but by the time it reaches customeElements.define(tag, this), all the tests have been executed and the jest framework tears down everything, resulting in the above NPE. It appears as if there are some async dependency loading issues going on involving TimePicker and/or its dependencies.

I have confirmed that 1.23.8 is the first release version that regressed our tests. I have reason to believe that the offending change is possibly #9573.

Workarounds attempted:
Manually overriding FeaturesRegistry.registerFeature to not add the feature "InputSuggestions" to the feature map appears to resolve the load dependency issue and fixes the test, though doing this doesn't seem like a good idea.

Not known if this is an issue in production or not.

Affected Component

No response

Expected Behaviour

No response

Isolated Example

No response

Steps to Reproduce

...

Log Output, Stack Trace or Screenshots

No response

Priority

None

UI5 Web Components Version

1.24

Browser

Chrome

Operating System

No response

Additional Context

No response

Organization

No response

Declaration

  • I’m not disclosing any internal or sensitive information.
@patrickwang10 patrickwang10 added the bug This issue is a bug in the code label Nov 14, 2024
@dobrinyonkov dobrinyonkov self-assigned this Nov 15, 2024
@nnaydenow
Copy link
Contributor

Hi @patrickwang10,

In version 1.24.x the feature need to be initialised before all components. We've tried to add support for dynamic imported featured but it wasn't stable and that's why we reverted it. So the features should work the same as 1.23.x, 1.22.x and etc.

@ui5/webcomponents package doesn't have version 1.23.8. Could you please check if this is react's one?

I tried to compare what are the changes between 1.23.x and 1.24.13 version of @ui5/webcomponents but there is no difference.

@patrickwang10
Copy link
Author

patrickwang10 commented Nov 15, 2024

Hi @nnaydenow

Can you elaborate a bit more on what you mean by the feature needs to be initialized before all components? Is there a guideline on best practices of initializing components before this dynamic import feature?

We are trying to upgrade "@ui5/webcomponents" to 1.24.13.

When I upgraded "@ui5/webcomponents" from 1.23.3 to 1.24.7, the tests passed
When I upgraded "@ui5/webcomponents" from 1.23.3 to 1.24.8+, the tests failed

On a second look at the code diff this time: v1.24.7...v1.24.8,
I noticed 800c188, which wasn't part of the release notes.
Could this be the problem? While playing around with this yesterday, I noticed that manually overriding FeaturesRegistry.registerFeature to not add the feature "InputSuggestions" to the feature map appears to also resolve our issue.

@dobrinyonkov
Copy link
Contributor

Hi @patrickwang10,

you can read more about the feature importing in the documentation here: https://sap.github.io/ui5-webcomponents/docs/advanced/using-features/

I tried reproducing the issue on your repository, but I couldn't. The command npm run jest-debug packages/sap/viz-design-panel/test/ui/feed/VarianceSection runs but it only outputs the following, no test execution starts:

➜  uqm git:(master) ✗ npm run jest-debug packages/sap/viz-design-panel/test/ui/feed/VarianceSection

> @sap/uqm@2022.1.0 jest-debug
> node --expose-gc --inspect-brk node_modules/jest/bin/jest.js --runInBand --config=./jest.config.js packages/sap/viz-design-panel/test/ui/feed/VarianceSection

Debugger listening on ws://127.0.0.1:9229/3d063abd-7941-4fa7-b060-7f68837a1c12
For help, see: https://nodejs.org/en/docs/inspector

Could you please provide more details on how to reproduce the issue?

Best regards,
Dobrin

@dobrinyonkov
Copy link
Contributor

Hi, @patrickwang10,

I managed to run the test and did some version comparison.

I checkout the repository at the commit before the one that updates the web components to 1.24.8 (commit) and run the tests. The tests passed.

Then I updated the version to 1.24.7 and run the tests. The tests passed.
Then I updated the version to 1.24.8 and run the tests. The tests passed.

Then I updated the version to the latest 1.24.13 and run the tests. The tests still passed.

I'm not sure what is the issue you are facing.
Could it be caused by something changed in the repository with the commit that updates the web components version?

Best regards,
Dobrin

@patrickwang10
Copy link
Author

patrickwang10 commented Nov 18, 2024

Hi @dobrinyonkov,

Thanks for looking into this.

We have merged our upgrade and disabled our tests temporarily. This will still be an issue while we try to re-enable the failing tests in the coming days. If you have just checked out to that commit (https://github.wdf.sap.corp/orca/uqm/pull/39980/files), you will need to re-enable the tests again (commented out) to reproduce the issue.

Thanks,
Patrick

@patrickwang10
Copy link
Author

Discussed with @nnaydenow offline. Confirmed that the tests passed after the upgrade if 800c188 is reverted. Possible that there are some underlying timing problems before that 800c188 has now exposed into a hard failure.

@nnaydenow
Copy link
Contributor

Hi Patric,

As we talked about offline, this looks like a timing issue with the event loop, not the web components themselves. It seems that Jest’s jsdom environment cleans up the document object before all the asynchronous tasks in the event loop are finished. Once the test ends, Jest removes the document and window objects during its cleanup.

This means the test wasn’t stable and only passed by chance.

In a normal browser, this kind of task wouldn’t run when the browser is closed, so there would be no issue. But in the test environment, the cleanup happens too early, which causes the problem.

Here are a few ideas to fix it:

  1. Change Jest Settings: Update Jest’s configuration so it waits for all tasks in the event loop to finish before cleaning up.

  2. Wait for Web Component Boot: Change the beforeAll hook to wait until the web components are fully loaded before running the test. For example:

    beforeAll(async () => {
        UI5.setupMocks({ mockConsoleError: true, mockFetch: true });
        await new Promise((resolve) => {
            attachBoot(() => {
                resolve();
            });
        });
    });

Let me know if you want to discuss these options or explore other solutions.

Regards,
Nayden

@patrickwang10
Copy link
Author

Hi @nnaydenow,

The majority of our tests have been updated using approach #2. And as discussed, this is a test only issue, so closing this bug now.

Thanks,
Patrick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author action bug This issue is a bug in the code
Projects
None yet
Development

No branches or pull requests

3 participants