Repository navigation
Support WarpDrive schema records as changeset content - #210
Open
Sadek-1801 wants to merge 4 commits into
Open
Sadek-1801 wants to merge 4 commits into
Sadek-1801 wants to merge 4 commits into
Conversation
Reading an unknown property throws on some objects, such as WarpDrive schema records, so isChangeset threw for every record it was given.
The changeset Proxy called key.toString() on every key. Ember's get reads the symbol PROXY_CONTENT, which became the string "Symbol(PROXY_CONTENT)" and was looked up on the content. Setting a symbol key also recorded a change.
A key missing from the changes and the changeset was always read off the content. Content that throws on unknown keys, such as WarpDrive schema records, broke get, set, save and validation. contentHasKey guards every content read. Content with its own unknownProperty, such as an Ember ObjectProxy, counts as having every key, since in cannot see the keys it forwards. Subclasses can override contentHasKey for other content like that.
get() read the changed value at the key's remaining path even when there was none, so it asked the value for a field named ''. A plain object answers undefined; a strict record, such as a WarpDrive schema record, throws.
| @@ -0,0 +1,12 @@ | |||
| // Like a WarpDrive schema record: reading a string key outside its schema | |||
| // throws, while symbol reads and `in` answer without throwing. | |||
| export default function strictRecord(fields: Record<string, unknown>) { | |||
Contributor
There was a problem hiding this comment.
@Sadek-1801 do you think you could create an actual use case where we use WarpDrive schema? that would help proof it works and long term help with maintainability so that we could rely on this as a true regression setup rather than mock we need assume matches how the WarpDrive creates schemas?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes changesets work with content that throws on unknown keys, such as
WarpDrive schema records. Part of fixing
adopted-ember-addons/ember-changeset#710.
The problem
In development builds, a WarpDrive schema record throws
No field named X on <type>when code reads a property outside its schema.A changeset reads several such properties off its content, so passing a
schema record to
Changeset()fails at the first read.What changes
Each commit fixes one read and adds a failing test first.
isChangesetchecks'__changeset__' in objbefore reading it.Before, it threw for every schema record it was given.
Proxycalledkey.toString()on every key. Ember'sgetreads the symbolPROXY_CONTENT, which became the string"Symbol(PROXY_CONTENT)"and waslooked up on the content. Setting a symbol key also recorded a change.
Both
Proxyhandlers now share one handler insrc/utils/proxy-handler.ts.from the changes was always read off the content. That broke
get,set,saveand validation. Every content read now goes throughcontentHasKey(content, key). It useskey in content. Content with itsown
unknownProperty, such as an EmberObjectProxy, counts as havingevery key, since
incannot see the keys it forwards.get()skips the nested read when a changed key has no path left.It read the changed value at the path
'', which a schema record throws on.Behaviour change to review
Plain objects, class instances, Ember objects, Ember proxies and ember-data
models all behave as before.
Content that is a JavaScript
Proxywith agettrap but nohastrap doesnot. Before, such content returned its values. Now it returns
undefined.A subclass can override
contentHasKeyfor content like that.This may count as a breaking change. I leave the version bump to you.
The changeset also gains two methods,
contentHasKeyand_getContent. Acontent field with either name is now hidden behind the method, as a field
named
saveorvalidatealready is.Testing
pnpm test: 416 passed, 2 skipped (the same 2 as onmain).pnpm lintandtscare clean, and each commit passes on its own.master, unchanged, linked to this branch: 200 passed,1 todo, 0 failed.
The
compatCI job will probably fail, but not because of this change.bin/test-external-changeset.jsrunscd ember-changesetinside the clone,but the add-on is now at the repository root. The clone is also tested before
its
distis built. I ran ember-changeset's suite by hand instead, as above.