Skip to content

Conversation

@joachimvh
Copy link
Member

As discussed in #105 (reply in thread)

I used the componentResources as an additional parameter since that is what ConfigPreprocessorComponent did, but I'm not actually sure why/if they're needed since all the resources with the same identifier seem to be the same everywhere. Using the type itself instead of componentResources[type.value] seemed to also work.

Also noticed there was a file with URI constants so used that for the override URIs.

@joachimvh joachimvh requested a review from rubensworks August 23, 2022 12:26
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2911512099

  • 0 of 15 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2796194601: 0.0%
Covered Lines: 1362
Relevant Lines: 1362

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2911512099

  • 15 of 15 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2796194601: 0.0%
Covered Lines: 1362
Relevant Lines: 1362

💛 - Coveralls

@rubensworks rubensworks merged commit 7f06aa1 into master Aug 23, 2022
@rubensworks
Copy link
Member

Looks perfect! Released as 5.3.1.

I used the componentResources as an additional parameter since that is what ConfigPreprocessorComponent did, but I'm not actually sure why/if they're needed since all the resources with the same identifier seem to be the same everywhere. Using the type itself instead of componentResources[type.value] seemed to also work.

If I remember correctly, this had something to do with resources being created by different object loaders, and that possibly causing issues sometimes.

@joachimvh joachimvh deleted the fix/override branch August 23, 2022 13:29
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.

4 participants