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

Defining built-in properties as non-enumerable. #282

Closed
wants to merge 3 commits into from

Conversation

gfoust
Copy link
Contributor

@gfoust gfoust commented Mar 2, 2016

I would like to be able to iterate over all data in a session, like so:

for (var key in req.session) { console.log(key + '=' + req.session[key]); }

Currently this is complicated by the fact that the session object contains non-data properties such as save, touch, and cookie. This patch simply defines those properties as non-enumerable using Object.defineProperty so that the above iteration technique works correctly.

@dougwilson
Copy link
Contributor

Thanks! It looks like this change caused tests to fail if you can take a look, please :)

@dougwilson
Copy link
Contributor

In addition for taking a look into the failing tests this change caused, please also add at least one test that verifies the changes you made are working, otherwise your changes are likely to be un-done by another person and no one will noticed it's a problem since nothing will fail.

@gfoust
Copy link
Contributor Author

gfoust commented Mar 2, 2016

Will do, thanks.

@gfoust
Copy link
Contributor Author

gfoust commented Mar 2, 2016

I see now that MemoryStore depends on "cookie" being enumerable (which means other stores probably do as well). I reverted that change; now only methods are non-enumerable.

I added a test case for the change.

@dougwilson
Copy link
Contributor

Awesome! Can you make those properties configurable, so users can redefine them, if there were doing that for some reason previously?

@gfoust
Copy link
Contributor Author

gfoust commented Mar 4, 2016

I made them configurable. I'm not sure if this was what was desired, but I also made them writable, since they were writable previously. Thus, the only change introduced by this patch is making them non-enumerable.

@dougwilson dougwilson self-assigned this Mar 7, 2016
@dougwilson
Copy link
Contributor

Thank you so much! Yea, I'm not sure that they need to be configurable, but I just don't quite know what people are doing out there, and figured that getting this in a minor version that would at least be one less possible break :)

@dougwilson dougwilson closed this in 30a3f49 Mar 7, 2016
dougwilson pushed a commit that referenced this pull request Jun 13, 2016
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.

None yet

2 participants