-
Notifications
You must be signed in to change notification settings - Fork 12
Give every FileDef subtype module a default export; name the field in thunk errors #5802
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
Open
lukemelia
wants to merge
2
commits into
main
Choose a base branch
from
cs-12169-filedef-subtype-modules-have-inconsistent-export-shapes-some
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,3 +39,5 @@ export class AvifDef extends RasterImageDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default AvifDef; | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -463,3 +463,5 @@ export class CsvFileDef extends FileDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default CsvFileDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,3 +54,5 @@ export class FlacDef extends AudioDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default FlacDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,3 +43,5 @@ export class GifDef extends RasterImageDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default GifDef; | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,3 +47,5 @@ export class JpgDef extends RasterImageDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default JpgDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -594,3 +594,5 @@ export class JsonFileDef extends FileDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default JsonFileDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,3 +55,5 @@ export class M4aDef extends AudioDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default M4aDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -558,3 +558,5 @@ export class MarkdownDef extends FileDef { | |
| return attributes; | ||
| } | ||
| } | ||
|
|
||
| export default MarkdownDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,3 +102,5 @@ export class Mp3Def extends AudioDef { | |
| } | ||
| } | ||
| } | ||
|
|
||
| export default Mp3Def; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,3 +55,5 @@ export class OggDef extends AudioDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default OggDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,3 +43,5 @@ export class PngDef extends RasterImageDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default PngDef; | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,3 +243,5 @@ export class TextFileDef extends FileDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default TextFileDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -303,3 +303,5 @@ export class TsFileDef extends FileDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default TsFileDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,3 +75,5 @@ export class WavDef extends AudioDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default WavDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,3 +39,5 @@ export class WebpDef extends RasterImageDef { | |
| }; | ||
| } | ||
| } | ||
|
|
||
| export default WebpDef; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { getService } from '@universal-ember/test-support'; | ||
| import { module, test } from 'qunit'; | ||
|
|
||
| import { setupRenderingTest } from '../helpers/setup'; | ||
|
|
||
| // When a field's card class evaluates to undefined — most often a default | ||
| // import of a module that only has named exports, or a genuine cycle — the | ||
| // thrown error names the field and its owning card and spells out both | ||
| // causes, so the author lands on the exact declaration instead of bisecting | ||
| // the schema. The eager form throws at decoration time; the thunk form | ||
| // throws at the first `field.card` read. These call the `field` decorator | ||
| // the same way Babel's decorator transform does. | ||
| module('Unit | card field thunk errors', function (hooks) { | ||
| setupRenderingTest(hooks); | ||
|
|
||
| let fieldKinds = (api: any): [string, string, (value: any) => unknown][] => [ | ||
| ['linksTo', 'src', api.linksTo], | ||
| ['linksToMany', 'links', api.linksToMany], | ||
| ['contains', 'meta', api.contains], | ||
| ['containsMany', 'items', api.containsMany], | ||
| ]; | ||
|
|
||
| test('an undefined field card class names the field, the owner, and both causes', async function (assert) { | ||
| let loader = getService('loader-service').loader; | ||
| let api = (await loader.import('@cardstack/base/card-api')) as any; | ||
| class Broken extends api.CardDef {} | ||
|
|
||
| for (let [label, fieldName, fieldFn] of fieldKinds(api)) { | ||
| assert.throws( | ||
| () => | ||
| api.field(Broken.prototype, fieldName, { | ||
| initializer: () => fieldFn(undefined), | ||
| }), | ||
| (err: Error) => | ||
| new RegExp(`field '${fieldName}' on 'Broken'`).test(err.message) && | ||
| /export shape/.test(err.message) && | ||
| /cyclic dependency/.test(err.message), | ||
| `${label} names the field, the owner, and both causes`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| test('a thunk resolving to undefined names the field at first read', async function (assert) { | ||
| let loader = getService('loader-service').loader; | ||
| let api = (await loader.import('@cardstack/base/card-api')) as any; | ||
| let { isField } = (await loader.import('@cardstack/runtime-common')) as any; | ||
| class BrokenThunk extends api.CardDef {} | ||
|
|
||
| for (let [label, fieldName, fieldFn] of fieldKinds(api)) { | ||
| // The thunk defers evaluation, so decoration itself succeeds. The | ||
| // decorator returns the property descriptor whose getter carries the | ||
| // Field object; `field.card` is the accessor every consumer reads | ||
| // through. | ||
| let descriptor = api.field(BrokenThunk.prototype, fieldName, { | ||
| initializer: () => fieldFn(() => undefined), | ||
| }); | ||
| let field = (descriptor?.get as any)?.[isField]; | ||
| assert.ok(field, `${label} field is registered`); | ||
| // …and the named error surfaces the first time the class is needed. | ||
| assert.throws( | ||
| () => field.card, | ||
| (err: Error) => | ||
| new RegExp(`field '${fieldName}' on 'BrokenThunk'`).test( | ||
| err.message, | ||
| ) && | ||
| /export shape/.test(err.message) && | ||
| /cyclic dependency/.test(err.message), | ||
| `${label} thunk form names the field and owner at first read`, | ||
| ); | ||
|
lukemelia marked this conversation as resolved.
|
||
| } | ||
| }); | ||
| }); | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import { getService } from '@universal-ember/test-support'; | ||
| import { module, test } from 'qunit'; | ||
|
|
||
| import { | ||
| FILEDEF_CODE_REF_BY_EXTENSION, | ||
| baseRealm, | ||
| } from '@cardstack/runtime-common'; | ||
|
|
||
| import { setupRenderingTest } from '../helpers/setup'; | ||
|
|
||
| // Every FileDef module exports its def class both named and as the default, | ||
| // so `import X from '…'` and `import { X } from '…'` are equally valid. A | ||
| // named-only module makes a default import silently evaluate to undefined, | ||
| // which then fails far away at schema time; a consistent shape removes the | ||
| // trap. Walking the extension registry keeps this guard covering every | ||
| // registered subtype, including ones added later. | ||
| // | ||
| // What this pins is "a default import never yields undefined" — not "a | ||
| // default import yields the class you named". A module carrying a second, | ||
| // named-only class is the sharper trap: `import RasterImageDef from | ||
| // './image-file-def'` evaluates to ImageDef (the default), a perfectly valid | ||
| // class, so nothing throws and the field is silently wired to the wrong | ||
| // class. image-file-def (RasterImageDef) and zip-file-def (ArchiveEntryField) | ||
| // are the two such modules today; only their registered class is pinned to | ||
| // the default below. | ||
| module('Unit | FileDef subtype export shapes', function (hooks) { | ||
| setupRenderingTest(hooks); | ||
|
|
||
| test('every registered subtype module has matching named and default exports', async function (assert) { | ||
| let loader = getService('loader-service').loader; | ||
| let seen = new Set<string>(); | ||
| for (let ref of Object.values(FILEDEF_CODE_REF_BY_EXTENSION)) { | ||
| // Real entries are built by `baseModule()` in full-URL form; a bare | ||
| // relative specifier marks a synthetic test-only entry with no module | ||
| // behind it (`.mismatch` today). | ||
| if (!ref.module.includes('://')) { | ||
| continue; | ||
| } | ||
| let key = `${ref.module}#${ref.name}`; | ||
| if (seen.has(key)) { | ||
| continue; | ||
| } | ||
| seen.add(key); | ||
| let ns = (await loader.import(ref.module)) as Record<string, unknown>; | ||
| assert.ok(ns[ref.name], `${ref.module} has named export ${ref.name}`); | ||
| assert.ok(ns['default'], `${ref.module} has a default export`); | ||
|
lukemelia marked this conversation as resolved.
|
||
| // A module registered under exactly one class defaults to that class. | ||
| // A module that ever registers two leaf classes would default to one | ||
| // of them; there, existence of *a* default is all this walk can | ||
| // require (see the module comment for why that case is a trap of its | ||
| // own). | ||
| let namesInModule = new Set( | ||
| Object.values(FILEDEF_CODE_REF_BY_EXTENSION) | ||
| .filter((r) => r.module === ref.module) | ||
| .map((r) => r.name), | ||
| ); | ||
| if (namesInModule.size === 1) { | ||
| assert.strictEqual( | ||
| ns['default'], | ||
| ns[ref.name], | ||
| `${ref.module} default export is ${ref.name}`, | ||
| ); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // The registry maps extensions to leaf subtypes only, so the family's base | ||
| // modules — the ones a subtype author actually imports from — appear in no | ||
| // entry and the walk above never touches them. One of these defaults is | ||
| // already load-bearing in the shipped tree: svg-image-def default-imports | ||
| // image-file-def. | ||
| test('family base modules have matching named and default exports', async function (assert) { | ||
| let loader = getService('loader-service').loader; | ||
| let baseModules: { module: string; name: string }[] = [ | ||
| { module: 'image-file-def', name: 'ImageDef' }, | ||
| { module: 'audio-file-def', name: 'AudioDef' }, | ||
| { module: 'video-file-def', name: 'VideoDef' }, | ||
| { module: 'font-file-def', name: 'FontDef' }, | ||
| { module: 'three-d-model-def', name: 'ThreeDModelDef' }, | ||
| { module: 'file-api', name: 'FileDef' }, | ||
| ]; | ||
| for (let { module: moduleName, name } of baseModules) { | ||
| let moduleId = `${baseRealm.url}${moduleName}`; | ||
| let ns = (await loader.import(moduleId)) as Record<string, unknown>; | ||
| assert.ok(ns[name], `${moduleId} has named export ${name}`); | ||
| assert.ok(ns['default'], `${moduleId} has a default export`); | ||
| assert.strictEqual( | ||
| ns['default'], | ||
| ns[name], | ||
| `${moduleId} default export is ${name}`, | ||
| ); | ||
| } | ||
| }); | ||
| }); | ||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.