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

warn if a mismatched global session is present (fix #47) #52

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

Conversation

runspired
Copy link
Collaborator

No description provided.

@runspired
Copy link
Collaborator Author

cc @hjdivad @trentmwillis

// The name of the property encodes the session/node compatibilty version
if (!global._heimdall_session_3) {
global._heimdall_session_3 = new Session();
}

for (let i = 1; i < 3; i++) {
Copy link

Choose a reason for hiding this comment

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

Might be unnecessary, but we could do const CURRENT_VERSION = 3; in the module scope and replace 3 everywhere with the constant so that future upgrades are easy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, makes what's going on here more intuitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Basic approach seems fine although TBH i think it would be clearer to just unroll the loop. We really don't plan on bumping the session version often.

if ('_heimdall_session_1' in global || '_heimdall_session_2' in global) {
  //warn
}

Also tests.

global[CURRENT_SESSION_KEY] = new Session();
}

for (let i = 1; i < CURRENT_VERSION; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

needs test

@stefanpenner
Copy link
Member

stefanpenner commented Apr 4, 2017

This may have gone stale, do we want this still?

@hjdivad
Copy link
Collaborator

hjdivad commented Apr 4, 2017

@stefanpenner i think we do; it should be an indication to upgrade certain deps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants