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

refactor: remove vue [do not merge] #534

Merged
merged 12 commits into from
Mar 11, 2024
Merged

refactor: remove vue [do not merge] #534

merged 12 commits into from
Mar 11, 2024

Conversation

hwiem
Copy link
Collaborator

@hwiem hwiem commented Nov 30, 2023

Allow using this library in vue 3 projects:

  • Make vue a peer dependency (>= 3) (it remains a dev dependency too, as it is used in the tests)
  • Make vuex a peer + dev dependency (>= 4, internal usages are removed, except for in tests)
  • Remove vue-template-compiler (only needed for vue 2)
  • Update jest and related dependencies to work with vue 3

Tasks

  • make the library work with vue 3 compat mode
  • fix tests (add vuex to dev dependencies, etc)

It shouldn't be neccessary as instance.
It should be available from outside
@hwiem hwiem added the enhancement New feature or request label Nov 30, 2023
@hwiem hwiem marked this pull request as draft November 30, 2023 09:40
- vuex and vue/compat are added to peerDependencies,
they are not needed as regular dependencies
- vue and vue-template-compiler (the latter needed for
vue 2, but not vue 3) could be removed as dependencies
- to be able to run the library in a vue 3 project,
vuex is required in ^v4
hwiem added a commit to demos-europe/demosplan-core that referenced this pull request Dec 1, 2023
- to work with vue 3, vuex needs to be updated to v4
- new Store() is now createStore()
- vue/compat and vue/compiler-sfc need to be updated
to make things work
- vue-template-compiler is only needed in vue 2, so
it can be removed
- dependencies need to be updated in demosplan-ui
and vuex-json-api, too:
  - demos-europe/demosplan-ui#658
  - eFrane/vuex-json-api#534
to be able to remove vuex as a direct dependency,
we need to remove its usages in the library
to be able to remove vue as a dependency,
we need to remove all its usages
these are needed for running the tests
- since state.items is an object, using `findIndex` on
it throws an error
- instead, we can just use `delete` to delete the item
@hwiem hwiem marked this pull request as ready for review December 5, 2023 11:08
test/apiMock.js Outdated Show resolved Hide resolved
test/setup.js Outdated Show resolved Hide resolved
@hwiem hwiem requested a review from salisdemos December 6, 2023 11:20
hwiem added a commit to demos-europe/demosplan-core that referenced this pull request Dec 6, 2023
* build(refs T19759): update vuex to v4

- to work with vue 3, vuex needs to be updated to v4
- new Store() is now createStore()
- vue/compat and vue/compiler-sfc need to be updated
to make things work
- vue-template-compiler is only needed in vue 2, so
it can be removed
- dependencies need to be updated in demosplan-ui
and vuex-json-api, too:
  - demos-europe/demosplan-ui#658
  - eFrane/vuex-json-api#534

* docs(refs T19759): remove redundant documentation comments

* refactor(refs T19759): declare emitted events

in vue 3, events emitted from a component need
to be declared via the 'emits' option

* refactor(refs T19759): define async components

in vue 3, dynamic imports need to be wrapped in a
`defineAsyncComponent()`

* build(refs T19759): disable devtools in prod mode

Co-authored-by: salisdemos <[email protected]>

AB#9149

---------

Co-authored-by: Florian Salis <[email protected]>
Co-authored-by: salisdemos <[email protected]>
@hwiem hwiem changed the title refactor: remove vue refactor: remove vue [do not merge] Dec 6, 2023
Copy link
Owner

@eFrane eFrane left a comment

Choose a reason for hiding this comment

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

Thank you for this massive piece of work!

I looked it through and besides bringing Vue 3 compat, this also makes the code so much more vanilla, which can only be a good thing in the long run.

The remaining question for me is, how do we merge this?

My proposal would be, to actually just throw it into main with a huge breaking disclaimer in the Changelog. There is enough of those already due to the whole fetch-refactoring from a while ago.

Versioning wise, we should probably go to 0.1.x with all of these changes. This would leave the 0.0.x branch open for any required fixes of this package in current production setups.

@hwiem @salisdemos wdyt?

@mussbach
Copy link

mussbach commented Jan 3, 2024

I would agree with you and prefer a 0.1.x as well

@salisdemos
Copy link
Collaborator

Sounds good for me. Totally agree.

@eFrane eFrane merged commit 8209b80 into main Mar 11, 2024
1 check passed
@eFrane eFrane deleted the f_update_vue branch March 11, 2024 16:55
eFrane added a commit that referenced this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants