Skip to content

Added ignoredPaths option - #22

Open
MaverickMartyn wants to merge 17 commits into
vue-electron:masterfrom
MaverickMartyn:master
Open

Added ignoredPaths option#22
MaverickMartyn wants to merge 17 commits into
vue-electron:masterfrom
MaverickMartyn:master

Conversation

@MaverickMartyn

Copy link
Copy Markdown

I added an option to supply an array of state paths to not persist.
This includes a unit test.
It simply works by cloning the state object, and deleting any properties that should not be persisted.
It then persists the modified clone instead,

I think this is my first actual pull request, so I hope it lives up to everyone's expectations.
Please let me know if something isn't working.

@Stormtv

Stormtv commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

@MaverickMartyn I played around with the changes in my local fork and everything seems to be working great. I'd recommend @MrEmelianenko to pull this in.

I think getting rid of the blacklist and whitelist might be a good idea since this solves their use cases. Also it is very dangerous to have bugs such as #19 when working with secure applications.

@MaverickMartyn

MaverickMartyn commented Feb 7, 2019

Copy link
Copy Markdown
Author

@Stormtv I agree that blacklist/whitelists should probably be removed, but I want o note this:
This PR does NOT solve all use cases of the blacklist and whitelist.
We could add an "inverted" option to cover the whitelist part, but one thing to note is that the store is still saved upon each commit, even if no "to-be-persisted"-data actually changes.
For example, in an app I am working on, I use the store to store the current playback time for a video.
This changes very often, and persisting the store on each commit, can cause IO issues.

Using the blacklist in combination with this, however works great.
I am thinking, maybe rename these things and redefine their purposes?

Maybe with an options structure like this:

{
    // Ignores specific paths on the state object, when persisting.
    ignoredPaths: ['test.testy', 'testy2.moretesty'],
    // Blocks commits from triggering a persistence update.
    ignoredCommits: ['TESTCOMMIT', 'ANOTHERCOMMIT'],
    // Changes the behavior from blacklist-like to whitelist-like. Default to false. Maybe another name?
    invertIgnored: true,
}

EDIT: Changed my proposal, to no longer suggest throwing an error, if inverting without supplying any of the other suggested params.

EDIT 2:
I now have a local test with all of this implemented, with unit tests as well.
It passes both the existing and my newly added unit tests at the moment.
Let me know if you want to take a look at it, and I'll update my master branch. :)

EDIT 3:
Pushed the changes mentioned in edit 2.

@MaverickMartyn

Copy link
Copy Markdown
Author

@MrEmelianenko @Stormtv Bump?

@NoelDavies

Copy link
Copy Markdown

@MaverickMartyn can you just return early? the else if's aren't needed and:

  1. Cause the codeclimate CI test to fail
  2. Add to cyclomatic complexity
  3. Add code smell

If you remove the else if's with just if's, it'll be nice :)

@NoelDavies

Copy link
Copy Markdown

@MaverickMartyn This is also a breaking change for some people, you'd need to keep the remaining functionality in there, and also you need to update the docs for your PR :)

@MaverickMartyn

Copy link
Copy Markdown
Author

I just couldn't be asked to fix the Code Climate issue at the time. :)
I'll take a look at it later, and implement your suggestions after I get home.

@akodkod

akodkod commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

@MaverickMartyn please fix all issues and I'll merge this into master. Thank you!

@MaverickMartyn

Copy link
Copy Markdown
Author

@MrEmelianenko One last minor issue: The bundle size is 2 kB and the file ./dist/persisted-state.js is now 2.17 kB.
I didn't want to change the bundle size limit without asking first, so should I just up the limit to 3?

@MaverickMartyn

Copy link
Copy Markdown
Author

@MrEmelianenko I just need to know if I can change the maximum size limit mentioned above.
Other than that, everything is done.

@Stormtv

Stormtv commented Mar 6, 2019

Copy link
Copy Markdown
Contributor

I personally think changing the max size to 3kB is fine. @MaverickMartyn but it would be nice is @MrEmelianenko would sign off on it :)

@MaverickMartyn

MaverickMartyn commented Apr 2, 2019

Copy link
Copy Markdown
Author

@Stormtv @MrEmelianenko I just accepted @Stormtv's pull request, which takes care of the bundle size limit. We should be good to go now.
Sorry about the slow response time. Life happens. :)

@Stormtv

Stormtv commented Apr 3, 2019

Copy link
Copy Markdown
Contributor

@MaverickMartyn @MrEmelianenko Turns out we did not need to increase the bundle size. I just made sure to minify the dist files in the PR which solved bundle size violation.

@Stormtv

Stormtv commented Apr 9, 2019

Copy link
Copy Markdown
Contributor

@MrEmelianenko Any update on this, would greatly appreciate this being included in vuex-electron.

@NoelDavies

Copy link
Copy Markdown

I have a need for this too. The ignore paths saved my bacon, but I'm now relying on a specific commit of the OPs PR

@dfranssen

Copy link
Copy Markdown

@MrEmelianenko poke

@Stormtv

Stormtv commented Apr 30, 2019

Copy link
Copy Markdown
Contributor

@MrEmelianenko Can we get this merged please?

@Stormtv

Stormtv commented May 8, 2019

Copy link
Copy Markdown
Contributor

@MrEmelianenko Are you alive <3

@Stormtv

Stormtv commented May 14, 2019

Copy link
Copy Markdown
Contributor

@MrEmelianenko Are there any issues with this PR?

@MaverickMartyn

Copy link
Copy Markdown
Author

@MrEmelianenko Just pinging you again, to make sure you see it. :)

@akodkod

akodkod commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

#44

Rhilip added a commit to Rhilip/IYUU-GUI that referenced this pull request Jul 17, 2020
vuex-electron存在很一个严重的问题。即使添加了blacklist,但是在下次非blacklist的commit的时候,还是会把所有的state进行持久化。鉴于vuex-electron已经不做维护,使用 MaverickMartyn/vuex-electron 作为替代

refs: vue-electron/vuex-electron#19
refs: vue-electron/vuex-electron#22
refs: https://github.com/MaverickMartyn/vuex-electron
@renaldasrep

Copy link
Copy Markdown

ping, would love to see this merged

@liuestc

liuestc commented Feb 26, 2021

Copy link
Copy Markdown

hi, my option is

const getMutationList = (mutation) => {
  let arr = [];

  for (const i in mutation) {
    arr.push(i);
  }

  return arr;
};
const DeviceMutationList = getMutationList(Device.mutations);

    createPersistedState({
      invertIgnored: true,
      ignoredCommits: DeviceMutationList,
      ignoredPaths: [
        'Device.currentSelected',
        'Device.deviceList',
        'state.Device.deviceList',
        'deviceList',
        'currentSelected',
      ],
    }),

my state is

{
	"state": {
		"Device": {
			"deviceList": [],
			"currentScanDeviceName": "MockPhoneName",
			"currentReconnectDeviceName": "",
			"showCurrentSelectedDeviceOffLine": false,
			"showDeviceAddingSucceed": true,
			"showDeviceAlreadyExist": false,
			"showDeviceReconnectedFailed": false,
			"showCreateDeviceConnectionFailed": false,
			"showDeviceAdding": false,
			"accountList": []
		},
		"Keyword": {
			"keyword": "",
			"needSearch": false
		},
		"QrCode": {
			"qrCode": {
				"ip": "172.23.73.103",
				"port": 9541,
				"createTime": 1614308688995
			}
		},
		"Setting": {
			"videoQuality": "high",
			"gameMode": false,
			"socketAddress": "ws://127.0.0.1:9541",
			"arsTokenPort": 9544,
			"quality": []
		},
		"Counter": {
			"main": 52
		}
	}
}

I don't want to persite the Device info in file, is there my config option error?
I see the vuex.json file have the Device info, how to i config the option,
Looking forward to your answer
@MrEmelianenko

@MaverickMartyn

Copy link
Copy Markdown
Author

hi, my option is

const getMutationList = (mutation) => {
  let arr = [];

  for (const i in mutation) {
    arr.push(i);
  }

  return arr;
};
const DeviceMutationList = getMutationList(Device.mutations);

    createPersistedState({
      invertIgnored: true,
      ignoredCommits: DeviceMutationList,
      ignoredPaths: [
        'Device.currentSelected',
        'Device.deviceList',
        'state.Device.deviceList',
        'deviceList',
        'currentSelected',
      ],
    }),

my state is

{
	"state": {
		"Device": {
			"deviceList": [],
			"currentScanDeviceName": "MockPhoneName",
			"currentReconnectDeviceName": "",
			"showCurrentSelectedDeviceOffLine": false,
			"showDeviceAddingSucceed": true,
			"showDeviceAlreadyExist": false,
			"showDeviceReconnectedFailed": false,
			"showCreateDeviceConnectionFailed": false,
			"showDeviceAdding": false,
			"accountList": []
		},
		"Keyword": {
			"keyword": "",
			"needSearch": false
		},
		"QrCode": {
			"qrCode": {
				"ip": "172.23.73.103",
				"port": 9541,
				"createTime": 1614308688995
			}
		},
		"Setting": {
			"videoQuality": "high",
			"gameMode": false,
			"socketAddress": "ws://127.0.0.1:9541",
			"arsTokenPort": 9544,
			"quality": []
		},
		"Counter": {
			"main": 52
		}
	}
}

I don't want to persite the Device info in file, is there my config option error?
I see the vuex.json file have the Device info, how to i config the option,
Looking forward to your answer
@MrEmelianenko

Hey there guys.

I am fairly sure this repository is no longer being maintained.
I have considered taking over, but I am hard pressed for time at the moment.
While I am unlikely to spend much time maintaining it, feel free to use my fork, if you want.

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.

7 participants