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

What's not production ready about this? #107

Open
mfbx9da4 opened this issue Aug 25, 2024 · 7 comments
Open

What's not production ready about this? #107

mfbx9da4 opened this issue Aug 25, 2024 · 7 comments

Comments

@mfbx9da4
Copy link

This is an experimental and explicitly not production ready

As far as I can see, this is well tested and well written. What about this isn't ready yet?

@danieltroger
Copy link

We're using it in production and no issues so far

@nicolo-ribaudo
Copy link
Contributor

This is a polyfill for a TC39 proposal that is still in stage 2, and thus it could significantly change before being shipped in browsers.

@danieltroger
Copy link

danieltroger commented Aug 26, 2024

@nicolo-ribaudo ahh I see. Btw is the proposal dead? I've started to really love records and tuples in its current form, they solve a lot of problems. But since this repo hasn't been changed in 2 years and the proposal hasn't in 8 months, it's hard to feel hope?

I'm totally fine with the current state, the only thing that's annoying is converting a backend response that contains nested objects to a structure with records/tuples. I had to write a function that recursively makes things records, would be nice if there was a built-in way.

So if this could be merged into JavaScript and we'd get proper TypeScript support that would be amazing 🤞

@acutmore
Copy link
Contributor

What about this isn't ready yet?

Performance is likely to be the largest one. See #59. The implementation takes a general approach to all R&T - focusing on correctness rather than speed.

@acutmore
Copy link
Contributor

acutmore commented Sep 9, 2024

We're using it in production and no issues so far

Hi @danieltroger is this usage in a public repo? I'd love to have a look to see how R&T are helping

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 9, 2024

So I don't think the implementation could be much better optimized. At some point whether you use Immutable or whatever else you still have to look at every key-value in the worst case scenario. This like the other libraries has a strategy for early exiting.

If your anything like me you use a validator like Zod so you'll have to do a full pass on the data anyway so what's two passes?

At least with an immutable data structure you do the pass once rather than littered everywhere throughout the code base.

@danieltroger
Copy link

danieltroger commented Sep 10, 2024

Hi @danieltroger is this usage in a public repo? I'd love to have a look to see how R&T are helping

@acutmore: No, unfortunately not in a public repo :/ But we do have public source maps that you can use to read all the code :)
We're using it the drag-and drop logic for this shopify app: https://apps.shopify.com/depict.

  1. Set up a shopify dev store https://help.shopify.com/en/partners/dashboard/managing-stores/development-stores#create-a-development-store-for-testing-apps-or-themes
  2. Follow the link to the app above and install it
  3. Ensure you have sourcemaps enabled, in chrome devtools you can press cmd+shift+p and search for "sourcemaps"
  4. Find this file src/lite/views/Collection/DragAndDrop/useZone.tsx where it says const record
  5. Look at the code that now uses record, would be more painful without R&T

Basically that one and DragAndDropZones.tsx besides it is where we get the most benefits from R&T.

Screenshot 2024-09-10 at 13 55 22

We're also using it in this other file, but it's contested (the whole logic is). We're trying to diff local state vs server state there. To know if the user made any changes. It's fragile because false positives might come from random stuff that's in the state that we don't care about, and if we would revert to our pre-R&T approach where one manually compares all interesting properties false negatives are possible as things are added to the state but the comparison logic isn't updated.

Seems like it works quite well now so it might survive for a while more but the "less-complexity" approach is to just count any change as "a change was made" (disadvantage that we might indicate changes were made when they weren't)

Screenshot 2024-09-10 at 13 56 42

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

4 participants