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: timeout from config during late inject #552

Merged
merged 14 commits into from
Nov 15, 2023

Conversation

nair-sumesh
Copy link
Contributor

No description provided.

@nair-sumesh
Copy link
Contributor Author

Resolves Issue

@nair-sumesh
Copy link
Contributor Author

@vobu : could you please generate the package-lock file for the rest of the test to be executed. At my end it will add internal library dependency like last time..

@vobu
Copy link
Contributor

vobu commented Oct 12, 2023

@vobu : could you please generate the package-lock file for the rest of the test to be executed. At my end it will add internal library dependency like last time..

sure, will do.
but before: could you also please add a test for verifying the caching of the wdi5 config?
I'd suggest to add a dedicated cache-config.test.ts (or similar name) to /test and test for late wdi5 injection containing the cached config

@nair-sumesh
Copy link
Contributor Author

@vobu : could you please generate the package-lock file for the rest of the test to be executed. At my end it will add internal library dependency like last time..

sure, will do. but before: could you also please add a test for verifying the caching of the wdi5 config? I'd suggest to add a dedicated cache-config.test.ts (or similar name) to /test and test for late wdi5 injection containing the cached config

Done. let me know if this works.

@vobu
Copy link
Contributor

vobu commented Oct 13, 2023

@vobu : could you please generate the package-lock file for the rest of the test to be executed. At my end it will add internal library dependency like last time..

sure, will do. but before: could you also please add a test for verifying the caching of the wdi5 config? I'd suggest to add a dedicated cache-config.test.ts (or similar name) to /test and test for late wdi5 injection containing the cached config

Done. let me know if this works.

cool, thanks for adding the test → this safeguards the caching feature!
could you now please look into the failing tests? ignore the auth tests for now, but please peek into the failing JS- and TS-tests.
thx again!

@nair-sumesh
Copy link
Contributor Author

feature!

Actually I am not able to execute the test. I will try again at weekend.
I installed the dependency using the script in package.json.
However, during test execution, I get the error that ui5 service module is not found.
It worked last time, but not sure what's went wrong now.

@vobu
Copy link
Contributor

vobu commented Oct 16, 2023

feature!

Actually I am not able to execute the test. I will try again at weekend. I installed the dependency using the script in package.json. However, during test execution, I get the error that ui5 service module is not found. It worked last time, but not sure what's went wrong now.

don't forget to npm run build before working on any tests 😉
(or npm run build:watch)

the JS tests seem to be fixed now - good job!
however, there's still the TS test for late injecting UI5 failing - could you please have a look again! thx!

@nair-sumesh
Copy link
Contributor Author

should work now

@vobu
Copy link
Contributor

vobu commented Oct 16, 2023

should work now

cool, again thx!
btw: the one failing test in the auth test suite is fine IMO as it is one of the flaky ones...

now, @Siolto and @dominikfeininger - would like your review on this. We talked about caching the (wd5-) config before. Now there's a concrete use case for it. What do you think about this implementation?

Copy link
Collaborator

@dominikfeininger dominikfeininger left a comment

Choose a reason for hiding this comment

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

not sure about all the global.__wdi5Config though

src/lib/wdi5-bridge.ts Outdated Show resolved Hide resolved
src/service.ts Outdated Show resolved Hide resolved
@Siolto
Copy link
Collaborator

Siolto commented Nov 14, 2023

This PR is fine for from my side. Thanks @nair-sumesh for the input!
But we should generally check again were we declare all our different wdi5 specific properties. These are currently set in different places. A uniform location would be better here

@vobu
Copy link
Contributor

vobu commented Nov 14, 2023

so we're good to go it seems!
(i'll take a look at the failing auth test due to (again) package-lock issues)

@vobu vobu merged commit 2dc1e08 into ui5-community:main Nov 15, 2023
13 checks passed
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.

5 participants