★ wanayoo — archive 1999 https://github.com/yahoo/serialize-javascript/pull/27Nouvelle recherche | Portail wanayoo
Skip to content

/ serialize-javascript

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

Jump to bottom

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

@ethanresnick ethanresnick commented Mar 26, 2017

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

@yahoocla yahoocla commented Mar 26, 2017

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 ethanresnick force-pushed the ethanresnick:feature/date-continue branch from 7a37b13 to 5a06980 Mar 26, 2017
@ethanresnick
Copy link
Contributor Author

@ethanresnick ethanresnick commented Mar 26, 2017

CLA signed!
@ethanresnick ethanresnick force-pushed the ethanresnick:feature/date-continue branch from 5a06980 to 203dc6b Apr 2, 2017
@ethanresnick
Support dates
ba11f28 
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
Test using isJSON and space options together
Loading status checks…
3a13d54
@ethanresnick ethanresnick force-pushed the ethanresnick:feature/date-continue branch from 203dc6b to 3a13d54 Apr 2, 2017
@ethanresnick
Copy link
Contributor Author

@ethanresnick ethanresnick commented Jul 7, 2017

@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
caridy approved these changes Jul 14, 2017
View changes
@caridy
Copy link
Contributor

@caridy caridy commented Jul 14, 2017

potential maintainers: @okuryu @juandopazo
@okuryu
Copy link
Collaborator

@okuryu okuryu commented Jul 14, 2017

sure. I'll take a look it.
@okuryu
Copy link
Collaborator

@okuryu okuryu commented Jul 15, 2017

@redonkulus please add me into git and npm permissions.
@redonkulus
Copy link
Collaborator

@redonkulus redonkulus commented Jul 15, 2017

@okuryu done for both
@okuryu
Copy link
Collaborator

@okuryu okuryu commented Jul 15, 2017

Thanks!
@okuryu
okuryu approved these changes Jul 15, 2017
View changes
Copy link
Collaborator

@okuryu okuryu left a comment

👍
@okuryu okuryu merged commit 4cf1296 into yahoo:master Jul 15, 2017
2 checks passed
2 checks passed
cla/licenses User has a valid Yahoo CLA
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
This was referenced Jul 15, 2017
Closed
Closed
@xumx

This comment has been minimized.

Sign in to view
Copy link

@xumx xumx commented on ba11f28 Jul 20, 2017

changing from isRegExp to instanceof RegExp Caused breaking changes in my code.

function deserialize (serializedJavascript) {
  const vm = require('vm')
  let sandbox: any = {}
  vm.createContext(sandbox)
  vm.runInContext('var value = ' + serializedJavascript, sandbox)
  return sandbox.value
}

//This is false
deserialize('/abc/i') instanceof RegExp

//This returns {}
serialize(deserialize(serialize(/abc/i))

Is there a proper deserialize method that works?
@ethanresnick
Copy link
Contributor Author

@ethanresnick ethanresnick commented Jul 20, 2017 •
edited

@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.]
@ethanresnick ethanresnick mentioned this pull request Jul 20, 2017
Open
@caridy
Copy link
Contributor

@caridy 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

@ethanresnick ethanresnick commented Jul 21, 2017

Continuing this in #31...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okuryu okuryu

@caridy caridy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@ethanresnick @yahoocla @caridy @okuryu @redonkulus @xumx
You can’t perform that action at this time.