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

Don't require get/unwrap when working with config and objects in the environment #58

Merged
merged 10 commits into from
Mar 14, 2024

Conversation

dfreeman
Copy link
Member

@dfreeman dfreeman commented Mar 13, 2024

Background

Today, the config that every Exclaim component receives, as well as any object retrieved from the Exclaim environment, is wrapped in an EnvironmentArray or EnvironmentData shell that requires all properties are accessed via get, and wraps any returned values from that get in further shells.

When Exclaim was first built, all computed properties in Ember required get/set, so it wasn't really a big deal that our strategy for handling binding resolution required everything to go through those, since it just felt like any other normal Ember code. Today, though, with computed properties setting up native getters, and the distinction between wrapped and unwrapped data causing chaos with TS, being forced to use get everywhere and know when to unwrap things feels much worse and is a lot more confusing.

This Change

This change completely overhauls how we manage $bind and $helper in UI specs. Previously, when you called get on a field for the first time, if we discovered a Binding or HelperSpec in the underlying data, we would create a computed property on the fly for it and then get that. If we discovered any other non-primitive value, we would create a computed that returned that value wrapped in EnvironmentData so that it too would have this just-in-time computed installation set up.

This approach required the consumer call get to act as an intermediary and give us the opportunity to do that setup, so even after normal computeds stopped requiring get, it was still required for Exclaim objects.

With this change, we instead crawl the config once and set up the appropriate computed properties immediately, which means since the computeds themselves don't require get, and since we don't need to do anything special for nested values, we can stop wrapping everything as well.

Not This Change

There are a few notable things this change does not do:

  • Change Environment to not require get. The @env arg that components receive is kind of a weird hybrid of "pretends to be the @env arg that was passed to ExclaimUi" while also being "a grab bag of tools for managing the scope that $bind sees, and also weird metaForField stuff". To avoid this PR becoming overwhelming, I refrained from reworking that here, but I intend to take a pass at that as well before declaring 2.0.
  • Get rid of metaForField/resolveEnvMeta. After surveying our current usage, I think we should do that and replace it with a simple resolveEnvPath importable, which is the only real use case we ever actually had for field metadata. Likely makes sense as part of the environment changes mentioned above.
  • Modernize stuff (usage of @ember/component, @computed, etc)
  • Add an opt-in flag on ExclaimUi that uses unadorned getters for binding instead of computed() for compatibility with @tracked data. That may or may not happen before 2.0, but the idea would be that this opt-in mode becomes the only mode in 3.0 and acts as a path to incremental migration to @tracked in 2.x.
  • TypeScript. At a minimum for 2.0 I want to add canonical declarations here so we stop writing piecemeal ones everywhere, but it may make sense (we'll see after the refactoring dust settles) to actually just convert the source over; we'll see.

Copy link

@mnoble01 mnoble01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

Comment on lines +121 to +124
assert.strictEqual(get(this.config, 'baz'), 'bar');

set(this.env, 'foo', 'qux');
assert.strictEqual(get(this.config, 'baz'), 'qux');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .gets on the config object should be able to be removed now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! These tests were largely lifted whole-cloth from the other existing ones, just having the boilerplate refactored. My plan is to come through and get rid of all the get calls in the next PR where I clean up the Environment portion of things—then I won't have to comb through and think so hard about which gets are necessary and which aren't, I'll be able to just drop them all.

Comment on lines +185 to +186
const subvalue = get(value, 'key');
assert.strictEqual(get(subvalue, 'child'), 'bar');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these .gets still necessary for subvalues? Same question for the "array values" test below.

assert.strictEqual(get(ext2, 'a'), 'newbar');
assert.strictEqual(get(ext2, 'b'), undefined);

// Writing to an inherited key updates at the level it was inherited from

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are interesting. Can you explain when/why the environment is extended like this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now - it's done in components that render nested exclaim configuration.

@dfreeman dfreeman merged commit 5477b24 into main Mar 14, 2024
8 checks passed
@dfreeman dfreeman deleted the no-get branch March 14, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants