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

Encode Time objects as ISO8601 for compatibility with JS's Date.parse() #721

Closed
paddor opened this issue Dec 19, 2024 · 4 comments
Closed

Comments

@paddor
Copy link

paddor commented Dec 19, 2024

I've read about the recent performance gains in this gem. Congratulations!

It's nothing urgent but eventually I'd love to switch from Oj to this gem. Currently I can't do that because Time objects are not encoded as ISO8601, which is what Javascript's Date.parse() expects.

Specifically, I've been using these Oj options:

        OJ_OPTIONS = {
          mode:             :custom,
          symbol_keys:      true,
          time_format:      :xmlschema, # ISO 8601, compatible with JavaScript's Date.parse()
          allow_gc:         false,
          second_precision: 3,
          cache_keys:       true,
          cache_str:        35

          # NOTE: MUST NOT use use_to_json. Because some gem requires 'json' (and also the specs), Time#to_json gets defined
          #       and will be used by Oj. It's not ISO8601.
          # use_to_json:      true,
        }

Would it be possible to add options time_format: :xmlschema and second_precision: N to JSON.generate?

@paddor
Copy link
Author

paddor commented Dec 19, 2024

Actually I'm not even sure if this issue makes sense. Before generating JSON, I have to convert Ruby objects to Hashes anyway, so that's where I could convert Time objects to ISO8601 strings. But so far I've been relying on Oj to do this for me and it also converts any stray Time-objects that my #to_json_hash might not know about.

Let me know what you think. Thanks.

@paddor
Copy link
Author

paddor commented Dec 22, 2024

So I've been thinking a little bit about this. I've come up with something like this:

require 'json'
require 'time'

def sanitize(node)
  case node
  when Time
    node = node.iso8601(3)
  when Array
    node = node.map do |obj| 
      sanitize(obj)
    end
  when Hash
    node = node.transform_values do |value|
      sanitize(value)
    end
  end

  node
end


def dump(obj)
  ::JSON.dump sanitize(obj)
end

It works, but is obviously not very performant.

The other obvious solution would be:

class Time
  def to_json(*a)
    iso8601(3).to_json(*a)
  end
end

But since it's monkey patching, it just doesn't feel right.

Refinements can't be used because the limited scope. PR #718 seems to work on a solution for this problem.

@byroot
Copy link
Member

byroot commented Dec 27, 2024

But since it's monkey patching, it just doesn't feel right.

Yes, that's why I hate the to_json interface, it assume there is one and only one way to serialize a given class in JSON, hence the work on JSON::Encoder.

We could change the default behavior of Time#to_json, but it has been this way since so long that it's certain to cause a lot of problems to users, so I don't think it's a good idea. It's much better to move toward non-global APIs.

I leave this open for now and probably consider it solved once JSON::Encoder is finalized.

@paddor
Copy link
Author

paddor commented Dec 28, 2024

Thanks for the input. For now the the monkey patch will have to do. Closing this now and looking forward to the JSON::Encoder with great anticipation.

@paddor paddor closed this as completed Dec 28, 2024
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

No branches or pull requests

2 participants