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

Question for community about field accessibility #93

Closed
icambron opened this Issue Nov 22, 2017 · 1 comment

Comments

Projects
None yet
1 participant
@icambron
Member

icambron commented Nov 22, 2017

I'm about to make a release (edit: now released) and it contains a major change: Luxon no longer uses defineProperty or defineProperties to hold internal state and instead just uses regular old fields. You can see the change here. It made a huge perf difference in some of my perf testing. Like 6x for some simple stuff.

What I would like is some feedback about this. definePropert(y|ies) let Luxon hide state: it was a) much harder to mutate by accident and b) hid from callers that actually, some internal state has mutated. I'm more concerned about (b). Specifically, Luxon occasionally caches the results of computations. I've been adding and removing internal caching based on profiling results. That caching is easy and safe because Luxon's external API is immutable and those computations are idempotent. But when those fields are exposed it will look like they have mutated. I'm wondering if this is a problem for stuff like React naively looking at whether objects have changed by walking their fields, which are now visible, and deciding the object has mutated.

I could:

  1. Revert the change and use defineProperties despite its perf implications
  2. Keep the change but kill the caching, whatever their perf implications.
  3. Just keep the change.
  4. Do something else to hide fields

I'm going to release this today and cross my fingers. But if it breaks your app or if you know a better way to handle this, please comment here.

@icambron

This comment has been minimized.

Member

icambron commented Dec 6, 2017

Since literally no one has commented, I'm going with "no big deal".

@icambron icambron closed this Dec 6, 2017

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