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

Support Dates #27

Merged
merged 2 commits into from Jul 15, 2017
Merged

Support Dates #27

merged 2 commits into from Jul 15, 2017

Conversation

ethanresnick
Copy link
Contributor

This is a continuation of #16.

To detect Dates, it uses a variant of the most recent strategy proposed by @ericf at #16 (comment). Using that strategy also suggested some tweaks to make the type detection mechanism more robust if Function.prototype.toJSON or RegExp.prototype.toJSON has been set (see commit message).

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@ethanresnick
Copy link
Contributor Author

CLA signed!

We detect dates simply by doing an instanceof check on raw value being serialized.

We're also now looking at the typeof the *rawValue* not the toJSON()ed value
when determining whether a value is a function or a RegExp. This should allow
functions and regexs to be serialized properly even if a Function.prototype.toJSON
or a RegExp.prototype.toJSON has been added for some reason.

Finally, for consistency and because util.isRegExp has been deprecated, we also
update our logic for detecting RegExps to use the same strategy as for Dates.
@ethanresnick
Copy link
Contributor Author

@ericf Are you still involved with this library? If so, can you take a look at this? I'd love to see it merged!

@caridy
Copy link
Contributor

caridy commented Jul 14, 2017

potential maintainers: @okuryu @juandopazo

@okuryu
Copy link
Collaborator

okuryu commented Jul 14, 2017

sure. I'll take a look it.

@okuryu
Copy link
Collaborator

okuryu commented Jul 15, 2017

@redonkulus please add me into git and npm permissions.

@redonkulus
Copy link
Collaborator

@okuryu done for both

@okuryu
Copy link
Collaborator

okuryu commented Jul 15, 2017

Thanks!

Copy link
Collaborator

@okuryu okuryu left a comment

Choose a reason for hiding this comment

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

👍

@okuryu okuryu merged commit 4cf1296 into yahoo:master Jul 15, 2017
This was referenced Jul 15, 2017
@ethanresnick
Copy link
Contributor Author

ethanresnick commented Jul 20, 2017

@xumx If I understand correctly, the second serialize call is failing because the RegExp global in your sandbox is different than the RegExp global outside it, right? I.e., it's a cross-realm issue.

I see a few options here:

  1. We can switch from the instanceof Date and instanceof RegExp checks back to util.isDate and util.isRegExp. I'd moved away from those functions, though, because they're deprecated in Node (though I don't know the story behind why.)

  2. We can switch to user-land implementations of util.isDate and util.isRegExp that work around the cross-realm issue (idk how reliably), like https://github.com/ljharb/is-regex and https://github.com/ljharb/is-date-object

  3. You could pass the external RegExp and Date globals into your sandbox—ideally wrapped in a proxy somehow so that the sandbox can't mutate them—so that deserializing creates RegExps and Dates that share the same globals as the context where the deserialized value will be used. (Or just use eval.) Either of those approaches might be a security issue (idk really what the vm module's sandboxing gives you), but it would stop instanceof checks from failing in the code that uses the deserialized values, which is probably a good thing. [Also, as an aside, if this library intends to support deserialization with methods other than eval, it'd be good to have tests for that.]

@caridy
Copy link
Contributor

caridy commented Jul 21, 2017

Identity discontinuity is a real pain. I don't think providing expansion slots of any kind will help here because we also serialize functions, and if those functions are parsed and evaluated in a different realm, the objects that they produce (imagine that the function returns a regexp) will always exhibit different identity. The solution must be in user-land by evaluating the code in the correct sandbox, or work around the identity discontinuity problems.

Side Note: Unfortunately we don't have realms in the language yet (coming soon).

@ethanresnick
Copy link
Contributor Author

Continuing this in #31...

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.

None yet

6 participants