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

[full-ci] Use provide/inject for service injection #9222

Merged
merged 3 commits into from
Jun 15, 2023

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented Jun 13, 2023

Description

Use inject/provide for service injection instead of relying on the deprecated/discouraged getCurrentInstance

Related Issue

Motivation and Context

We want to enable extensions to use composables, for that we need to get rid of getCurrentInstance which is discouraged and not supported in that context.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Jun 13, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@dschmidt dschmidt force-pushed the use-provide-inject-for-services branch from 196f496 to ae7550a Compare June 13, 2023 21:06
app.use(abilitiesPlugin, createMongoAbility([]), { useGlobalProperties: true })

// TODO: move to announceArchiverService function
Copy link
Member Author

Choose a reason for hiding this comment

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

I will be juggling around quite a bit with init order, so I would like to tackle this in a follow up

@ownclouders
Copy link
Contributor

ownclouders commented Jun 13, 2023

Results for e2e-tests oCIS-2 https://drone.owncloud.com/owncloud/web/36517/13/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36517/tracing/download-space-alice-2023-6-13-10-52-42.zip

@dschmidt dschmidt changed the title Use provide/inject for service injection [full-ci] Use provide/inject for service injection Jun 13, 2023
@dschmidt dschmidt force-pushed the use-provide-inject-for-services branch 2 times, most recently from 0b97b8f to 722a270 Compare June 13, 2023 22:41
@dschmidt dschmidt force-pushed the use-provide-inject-for-services branch from 722a270 to 807ebb6 Compare June 13, 2023 23:46
@dschmidt dschmidt force-pushed the use-provide-inject-for-services branch from 807ebb6 to 445b06f Compare June 14, 2023 00:13
@@ -59,12 +59,13 @@ function getMountedWrapper({ mocks = {}, files = [], loading = false } = {}) {
const storeOptions = { ...defaultStoreMockOptions }
const store = createStore(storeOptions)
return {
mocks: defaultMocks,
provide: defaultMocks,
Copy link
Member Author

Choose a reason for hiding this comment

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

Uhm

pnpm-lock.yaml Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 1 Code Smell

72.4% 72.4% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

thanks for digging through this 🏗️

@@ -74,6 +79,24 @@ export const bootstrapApp = async (configurationPath: string): Promise<void> =>
translations: merge(translations, customTranslations)
})
announceClientService({ app, runtimeConfiguration, configurationManager, store })

// TODO: move to announceArchiverService function
Copy link
Contributor

Choose a reason for hiding this comment

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

can you resolve this todo in this PR or as an immediate followup please?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, your comment about followup was not visible in my review anymore (probably because of history change). fine by me if you do that in a followup. but it's easily forgotten and not a hard task, so please better earlier than later ;-)

@dschmidt dschmidt merged commit b1bd60d into master Jun 15, 2023
@delete-merged-branch delete-merged-branch bot deleted the use-provide-inject-for-services branch June 15, 2023 10:10
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