fix(angular): use initialize function when setting up ionic angular to avoid config errors - #24004
Merged
Merged
Conversation
When karma runs each test case the window object persists. Ionic does some initial setup in angular first before handing over to core. On each test case in karma angular bootstraps itself again, during this bootstrap we set Ionic.config. On the first test case this is fine as Ionic core is also bootstrapped but on the second test case this overwrites legitimate config which causes parts of Ionic to fail as the config is no longer in the expected format. This fix works by only setting Ionic.config if it does not already exist. If it does exist then we do not replace it with an object thats not compatible with core. There may be better ways to solve this issue but I didn't want to change too much of ionic/angular so this felt like the safest change
Contributor
|
Thanks for the PR! We appreciate the work you put into making this. I pushed a commit to your branch with an alternative way of fixing this issue. Ionic React and Ionic Vue call an Once the build passes I will merge this PR. Thanks again! |
liamdebeasi
changed the title
Contributor
|
Here is a dev build if you are interested in testing: |
Contributor
|
Merged. Thank you! |
Contributor
Author
|
Hey. I gave the dev build a test and all seemed to work as expected, thanks for the merge 🥳 |
6 tasks
This was referenced Nov 1, 2021
This was referenced Dec 1, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
When angular bootstraps it sets
Ionic.configregardless of whether it is set or not. This is fine in normal applications such as serving in the browser where angular is only initialised once.However in karma test suites angular is initialised on every test case but the window object persists between all test cases which causes ionic/angular to overwrite the config with something that is not compatible with the core components. This has been reported in #22853 as config.get throwing warnings when using ion-segment but I guess the issue could be for any components that use the config object.
Issue Number: 22853
What is the new behavior?
Now during startup ionic/angular checks to see if
Ionic.configis set. If it is it does not do anything with that variable. This seems to solve the problem described in #22853 and I've not noticed any adverse effects when stepping through the event listeners in helpers that were previously failing because the config had been overwritten with another objectDoes this introduce a breaking change?
Other information
When inspecting Ionic.config on a second test case
PS: I wasn't able to find any tests for this part and running npm test gave me
angular no tests yet. If theres some tests I need to write let me know and also how I should run the test suite.