Skip to content

[4.0] Fix various instances where we were doing bad things in vuex#24751

Merged
wilsonge merged 2 commits intojoomla:4.0-devfrom
wilsonge:feature/vuex-strict
May 1, 2019
Merged

[4.0] Fix various instances where we were doing bad things in vuex#24751
wilsonge merged 2 commits intojoomla:4.0-devfrom
wilsonge:feature/vuex-strict

Conversation

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Apr 28, 2019

This is harder to test but is cleaning up some tech debt in the code base. In some places we were directly manipulating the vuex state, which as documented in the code shouldn't be done. I've cleaned that up to correctly use a mutator. To test ensure that the rename modal works before and after the patch in media manager

In addition if you now build media manager in dev mode (or watch which is dev mode by default) we now enable vuex strict mode (note as per the docs you shouldn't use strict mode in production https://vuex.vuejs.org/guide/strict.html). This means in the future when people do dev work they should get warnings when they do such bad things.

@dneukirchen are you able to do a code review here please

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Apr 28, 2019
@wilsonge wilsonge changed the title Fix various instances where we were doing bad things in mvuex [4.0] Fix various instances where we were doing bad things in vuex Apr 28, 2019
Copy link
Contributor

@dneukirchen dneukirchen left a comment

Choose a reason for hiding this comment

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

code lgtm.

only some notes/questions:

  1. Why is this needed? 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV)

  2. Imo we dont need 2 way binding for the renaming. It might seem nice in the first place, but iirc the inline change is hidden behind a modal backdrop, so we can simplify things with 1 way databinding aka:

 <input id="name" class="form-control" placeholder="Name"
                               :value="name" @input="rename"	                             
                               required autocomplete="off">	

// change computed property name to only get the name from store
// Add rename method, which will commit the change

  1. personal opinion: i do not like words like "do", "run", "execute", "perform" or "process" xxx in method or var names, so i would rename "perform_rename" to "rename" or "change_name"

  2. FYI: we had discussions about removing vuex from mm to avoid the additional complexity it brings to the table. What’s your opinion on that?

@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 29, 2019

Why is this needed? 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV)

I took it from the docs on the environment plugin here. It gave it's equivalent using the define plugin https://webpack.js.org/plugins/environment-plugin/ (and it seemed to work for me). I always have to google this one every time...

Imo we dont need 2 way binding for the renaming. It might seem nice in the first place, but iirc the inline change is hidden behind a modal backdrop, so we can simplify things with 1 way databinding aka:

We can but then rather than do anything on change. We should just wait for the user to hit save. And use the existing save function. And remove the whole responsiveness other than for giving feedback on validity.

personal opinion: i do not like words like "do", "run", "execute", "perform" or "process" xxx in method or var names, so i would rename "perform_rename" to "rename" or "change_name"

I have no preference on naming conventions. Done CHANGE_NAME (although obviously this will go entirely if we do the above which seems logical)

FYI: we had discussions about removing vuex from mm to avoid the additional complexity it brings to the table. What’s your opinion on that?

I don't think you'll save yourself much complexity because for things like showing the modals we'll have to pass deep nested functions through the application (also for example dealing with selected items interaction with the toggle all). And that will become a maintenance issue as well (especially as it's "bad practice" for any of these js apps).

@wilsonge wilsonge force-pushed the feature/vuex-strict branch from d44e642 to 3056794 Compare April 29, 2019 00:56
@wilsonge wilsonge force-pushed the feature/vuex-strict branch from ad094bb to 05ac62a Compare April 29, 2019 01:11
@wilsonge
Copy link
Contributor Author

OK Second commit removes everything and just does the update in the existing RENAME_SUCCESS mutation. Which means it's only updating the UI after the API gives a success which is also much better from a sanity perspective. Before now if the API response failed we'd updated the UI like it was a success.

@wilsonge wilsonge merged commit 4368e24 into joomla:4.0-dev May 1, 2019
@wilsonge wilsonge deleted the feature/vuex-strict branch May 1, 2019 21:32
@wilsonge wilsonge added this to the Joomla 4.0 milestone May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants