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 Memory-ish protocol #44

Merged
merged 5 commits into from
Dec 22, 2017
Merged

Add Memory-ish protocol #44

merged 5 commits into from
Dec 22, 2017

Conversation

jquense
Copy link
Member

@jquense jquense commented Dec 21, 2017

Admittedly this is more of a SessionProtocol, It’s probably easy to write a pure memory one in terms of the this, but meh

@jquense
Copy link
Member Author

jquense commented Dec 21, 2017

context: we use this in an Electron app where there is no url bar, the session storage is so ctrl-r works

@taion
Copy link
Contributor

taion commented Dec 22, 2017

I updated the code a bit. How does this look?


// Checking instanceof Array is okay because we make the array.
if (
stack instanceof Array &&
Copy link
Member Author

Choose a reason for hiding this comment

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

This is still weird :P what's wrong with isArray?

Copy link
Contributor

Choose a reason for hiding this comment

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

requires a polyfill for older browsers, no? i don't have babel-runtime here... not sure what the right thing is

Copy link
Member Author

Choose a reason for hiding this comment

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

how far back are you going? isArray is ie9+

...this._stack[this._index],
action: 'POP',
index: this._index,
delta,
Copy link
Member Author

Choose a reason for hiding this comment

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

So I originally did this sticking index and delta on location, but what's the value in doing so?

Copy link
Contributor

Choose a reason for hiding this comment

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

in principle for consistency with browser protocol... e.g. if you want to animate differently based on whether delta is positive or negative

not sure if index has any purpose... really just from remix-run/history#36 and remix-run/history#334

@taion
Copy link
Contributor

taion commented Dec 22, 2017

Good to go on my end

@jquense
Copy link
Member Author

jquense commented Dec 22, 2017

lgtm :shipit:

@taion taion merged commit 2c75c20 into master Dec 22, 2017
@taion taion deleted the memory-protocol branch December 22, 2017 19:54
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.

2 participants