kiliman / remix-typedjson

This package is a replacement for superjson to use in your Remix app. It handles a subset of types that `superjson` supports, but is faster and smaller.
MIT License
435 stars 19 forks source link

fix: don't break on dot notation clashes #23

Closed baker-travis closed 10 months ago

baker-travis commented 1 year ago

Here's my stab at solving this issue (#22 ). It's a restructure in how the meta object is passed, but it allows more flexibility since we know the exact keys for the object.

baker-travis commented 1 year ago

Hi @kiliman, we're wanting to use this package, but need this to be fixed. Could you take a look? We can fork if this doesn't match your vision here.

kiliman commented 1 year ago

Sorry for the late response. I've been slammed with work and haven't had a lot of free time. Anyway, after reviewing your code, is it possible to change the meta encoding to escape the . instead of making it an object? I'd like to minimize the payload if possible.

That is instead of

[
  {
    path: ["firstObject.deeper", "anotherProperty"],
    type: "date"
  }
]

Use this instead. Escape . with \

{
  "firstObject\.deeper.anotherProperty": "date"
}

Edit: sorry, put the \ in the wrong spot. Basically \. means a literal . and will not be used to split. The normal . is still the property accessor. So It's like using: ["firstObject.deeper"]["anotherProperty"]

baker-travis commented 1 year ago

Thanks @kiliman, I appreciate you giving it a review. Going along with your suggestion, and to remove all assumptions about the consumer's data, I took it a step further.

Instead of this:

{
  "firstObject\\.deeper.anotherProperty": "date"
}

It will be more like this:

{
  "firstObject\\.deeper..anotherProperty": "date"
}

Using .. to separate the different properties. Since we're replacing . with \., the only way to have .. is through a separator. With just escaping . in the property name and separating with ., the property could end with \ and inadvertently cause the property separator . to become escaped and appear as if its the same property. Is this making sense? It's a definite edge case, but using .. as the separator while escaping . in the property name ensures that we will never have a conflict.

Please take a look and let me know.

baker-travis commented 10 months ago

Hey @kiliman, just wanted to check back in on this. We'd really love to close this small gap needed for our use-case.

kiliman commented 10 months ago

This was fixed in v0.3.1 with a different implementation. Thanks for the PR.