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

Add support for migrations #83

Merged
merged 41 commits into from Sep 9, 2019

Conversation

rafaelramalho19
Copy link
Contributor

@rafaelramalho19 rafaelramalho19 commented Jul 22, 2019

Fixes #74


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@satazor
Copy link

satazor commented Jul 23, 2019

Is it possible to use, for example, store.set('internal.version', '1.2.3') ?

Do you mean for users? Then yes, but it should not be allowed. We should document that the __internal__ key is reserved and throw if it's used from the outside. I doubt anyone would ever encounter it though.

I mean for internal usage. If the user has set accessPropertiesByDotNotation to false, the internal migration code would fail, right?

I guess you have to create a internal _set function that doesn't care about key names, while the exposed pubic set ensures that keys can't start with __internal__.

@rafaelramalho19
Copy link
Contributor Author

Is it possible to use, for example, store.set('internal.version', '1.2.3') ?

Do you mean for users? Then yes, but it should not be allowed. We should document that the __internal__ key is reserved and throw if it's used from the outside. I doubt anyone would ever encounter it though.

I mean for internal usage. If the user has set accessPropertiesByDotNotation to false, the internal migration code would fail, right?

I guess you have to create a internal _set function that doesn't care about key names, while the exposed pubic set ensures that keys can't start with __internal__.

Done!

Since we currently only need it for the migrations, I only used dot notations for now. (This skips the validation for this._options.accessPropertiesByDotNotation and all the validations associated with it.

If you prefer I can modify the set function flow to accept an override mechanism for the accessPropertiesByDotNotation.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@rafaelramalho19
Copy link
Contributor Author

Hi @sindresorhus, can you provide any update on this PR? 😄

@sindresorhus sindresorhus changed the title Adds support for migrations Add support for migrations Sep 9, 2019
@sindresorhus sindresorhus merged commit 931ffce into sindresorhus:master Sep 9, 2019
@sindresorhus
Copy link
Owner

This is excellent. Thank you for working on this, @rafaelramalho19. And sorry for the delay. The summer got the best of me and I'm way behind on OSS work.

@sindresorhus
Copy link
Owner

I noticed that there are no tests or docs (and TS) for using a semver range in the migration keys. Would you be able to submit a follow-up PR for that?

@rafaelramalho19
Copy link
Contributor Author

It's okay. I hope you had a nice summer @sindresorhus 😄

I'll create a follow-up PR this week and tag you.

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.

Add store migrations
4 participants