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

[2365] Remove _initialize method #2366

Conversation

BobrImperator
Copy link
Collaborator

@BobrImperator BobrImperator commented Apr 1, 2022

closes #2365

Before

Screen.Recording.2022-04-01.at.15.36.53.mov

After

Screen.Recording.2022-04-01.at.15.38.32.mov

Copy link
Member

@marcoow marcoow left a comment

Choose a reason for hiding this comment

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

Hm, I think this adds a bit of hidden complexity since now the store as we get it from the container won't actually be correctly initialized unless you all _initialize and not all of the stores have that method etc.

Can we not move the stuff that the cookie stores does in _initialize back to its init?

@BobrImperator
Copy link
Collaborator Author

@marcoow No, because adaptive store needs to pass options before that happens, otherwise session-store:cookie could run with incorrect arguments.
I don't really see much we can do without some refactoring.

The code that caused it:
https://github.com/simplabs/ember-simple-auth/pull/2312/files#diff-db3487bdb1f8c84714dedc96d1cce936b66f804e8a3c58ebe9d88dfb883e50deR137

@marcoow
Copy link
Member

marcoow commented Apr 1, 2022

Can the adaptive store not set the options after getting the cookie store from the container so that the cookie store would init itself as it would do when used standalone in init but the adaptive store simply overrides those settings afterwards?

@BobrImperator BobrImperator force-pushed the 2365-cookie-store-not-initializing branch from 8d11e04 to 8175dc0 Compare April 4, 2022 14:01
@BobrImperator BobrImperator force-pushed the 2365-cookie-store-not-initializing branch from 8175dc0 to c3648eb Compare April 4, 2022 14:07
@BobrImperator BobrImperator changed the title [2365] internal-session to call _initialize on session-store during init [2365] Remove _initialize method Apr 4, 2022
@BobrImperator
Copy link
Collaborator Author

Indeed it seems like it does the trick and no tests are failing - though I swear that was the case.
I dunno what I saw before 😅

@BobrImperator BobrImperator merged commit 9ff7b3d into mainmatter:master Apr 12, 2022
@BobrImperator BobrImperator deleted the 2365-cookie-store-not-initializing branch April 5, 2023 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CookieStore is not initializing/syncing correctly
2 participants