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

Added ignoredPaths option #22

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

MaverickMartyn
Copy link

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
Copy link
Contributor

Stormtv commented Feb 4, 2019

@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
Copy link
Author

MaverickMartyn commented Feb 7, 2019

@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
Author

@MrEmelianenko @Stormtv Bump?

@NoelDavies
Copy link

@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

@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
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
Copy link
Contributor

akodkod commented Feb 28, 2019

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

@MaverickMartyn
Copy link
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
Author

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

@Stormtv
Copy link
Contributor

Stormtv commented Mar 6, 2019

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
Copy link
Author

MaverickMartyn commented Apr 2, 2019

@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
Copy link
Contributor

Stormtv commented Apr 3, 2019

@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
Copy link
Contributor

Stormtv commented Apr 9, 2019

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

@NoelDavies
Copy link

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

@MrEmelianenko poke

@Stormtv
Copy link
Contributor

Stormtv commented Apr 30, 2019

@MrEmelianenko Can we get this merged please?

@Stormtv
Copy link
Contributor

Stormtv commented May 8, 2019

@MrEmelianenko Are you alive <3

@Stormtv
Copy link
Contributor

Stormtv commented May 14, 2019

@MrEmelianenko Are there any issues with this PR?

@MaverickMartyn
Copy link
Author

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

@akodkod
Copy link
Contributor

akodkod commented Aug 29, 2019

#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

ping, would love to see this merged

@liuestc
Copy link

liuestc commented Feb 26, 2021

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
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.

None yet

7 participants