Skip to content

Proof-of-concept: Use the object path as the ID of radio choices #2664

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
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

benchristel
Copy link
Member

Issue: none

Test plan:

TODO

Copy link
Contributor

github-actions bot commented Jun 30, 2025

Size Change: +144 B (+0.03%)

Total Size: 496 kB

Filename Size Change
packages/perseus-core/dist/es/index.item-splitting.js 12.9 kB +52 B (+0.4%)
packages/perseus-core/dist/es/index.js 21.5 kB +58 B (+0.27%)
packages/perseus-editor/dist/es/index.js 92.5 kB +34 B (+0.04%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 20.7 kB
packages/keypad-context/dist/es/index.js 1 kB
packages/kmath/dist/es/index.js 5.98 kB
packages/math-input/dist/es/index.js 98.6 kB
packages/math-input/dist/es/strings.js 1.61 kB
packages/perseus-linter/dist/es/index.js 7.07 kB
packages/perseus-score/dist/es/index.js 9.34 kB
packages/perseus-utils/dist/es/index.js 403 B
packages/perseus/dist/es/index.js 209 kB
packages/perseus/dist/es/strings.js 7.56 kB
packages/pure-markdown/dist/es/index.js 1.39 kB
packages/simple-markdown/dist/es/index.js 6.71 kB

compressed-size-action

@@ -75,6 +75,7 @@ const parseRadioWidgetV2 = parseWidgetWithVersion(
isNoneOfTheAbove: optional(boolean),
// deprecated
widgets: parseWidgetsMapOrUndefined,
id: defaulted(string, (_, ctx) => ctx.getPath().join(".")),
Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to be totally sure the path is unique, we could use formatPath from object-path.ts here.

@benchristel
Copy link
Member Author

Closing this PR because I don't think we're going to go this route for assigning IDs to choices.

@benchristel benchristel reopened this Aug 6, 2025
@github-actions github-actions bot added the schema-change Attached to PRs when we detect Perseus Schema changes in it label Aug 6, 2025
Copy link
Contributor

github-actions bot commented Aug 6, 2025

🗄️ Schema Change: Changes Detected ⚠️

This PR contains critical changes to Perseus. Please review
the changes and note that you may need to coordinate
deployment of these changes with other teams at Khan Academy.

diff --unified /home/runner/work/_temp/branch-compare/base/schema.d.ts /home/runner/work/_temp/branch-compare/pr/schema.d.ts
--- /home/runner/work/_temp/branch-compare/base/schema.d.ts	2025-08-06 21:43:41.167034522 +0000
+++ /home/runner/work/_temp/branch-compare/pr/schema.d.ts	2025-08-06 21:43:28.897885333 +0000
@@ -1160,6 +1160,11 @@
     rationale?: string;
     correct?: boolean;
     isNoneOfTheAbove?: boolean;
+    /**
+     * An opaque string that uniquely identifies this radio choice within the
+     * assessment item or article.
+     */
+    id: string;
   };
   export type PerseusSorterWidgetOptions = {
     correct: string[];

Copy link
Contributor

github-actions bot commented Aug 6, 2025

🛠️ Item Splitting: Changes Detected ⚠️

This PR contains critical changes to Perseus. Please review
the changes and note that you may need to coordinate
deployment of these changes with other teams at Khan Academy.

diff --unified /home/runner/work/_temp/branch-compare/base/index.item-splitting.js /home/runner/work/_temp/branch-compare/pr/index.item-splitting.js
--- /home/runner/work/_temp/branch-compare/base/index.item-splitting.js	2025-08-06 21:44:09.799975059 +0000
+++ /home/runner/work/_temp/branch-compare/pr/index.item-splitting.js	2025-08-06 21:43:42.325873965 +0000
@@ -8,7 +8,7 @@
 
 function success(value){return {type:"success",value}}function failure(detail){return {type:"failure",detail}}function isFailure(result){return result.type==="failure"}function isSuccess(result){return result.type==="success"}function all(results,combineFailureDetails=a=>a){const values=[];const failureDetails=[];for(const result of results){if(result.type==="success"){values.push(result.value);}else {failureDetails.push(result.detail);}}if(failureDetails.length>0){return failure(failureDetails.reduce(combineFailureDetails))}return success(values)}
 
-class ErrorTrackingParseContext{failure(expected,badValue){return failure([{expected:wrapInArray(expected),badValue,path:this.path}])}forSubtree(key){return new ErrorTrackingParseContext([...this.path,key])}success(value){return success(value)}constructor(path){this.path=path;}}function wrapInArray(a){return Array.isArray(a)?a:[a]}
+class ErrorTrackingParseContext{failure(expected,badValue){return failure([{expected:wrapInArray(expected),badValue,path:this.path}])}getPath(){return this.path}forSubtree(key){return new ErrorTrackingParseContext([...this.path,key])}success(value){return success(value)}constructor(path){this.path=path;}}function wrapInArray(a){return Array.isArray(a)?a:[a]}
 
 function formatPath(path){return "(root)"+path.map(formatPathSegment).join("")}function formatPathSegment(segment){if(typeof segment==="string"){return validIdentifier.test(segment)?"."+segment:`[${JSON.stringify(segment)}]`}return `[${segment.toString()}]`}const validIdentifier=/^[A-Za-z$_][A-Za-z$_0-9]*$/;
 
@@ -48,7 +48,7 @@
 
 function union(parseBranch){return new UnionBuilder(parseBranch)}class UnionBuilder{or(newBranch){return new UnionBuilder(either(this.parser,newBranch))}constructor(parser){this.parser=parser;}}function either(parseA,parseB){return (rawValue,ctx)=>{const resultA=parseA(rawValue,ctx);if(isSuccess(resultA)){return resultA}return parseB(rawValue,ctx)}}
 
-function defaulted(parser,fallback){return (rawValue,ctx)=>{if(rawValue==null){return success(fallback(rawValue))}return parser(rawValue,ctx)}}
+function defaulted(parser,fallback){return (rawValue,ctx)=>{if(rawValue==null){return success(fallback(rawValue,ctx))}return parser(rawValue,ctx)}}
 
 const parsePerseusImageDetail=object({width:number,height:number});
 
@@ -70,11 +70,11 @@
 
 function deriveExtraKeys(widgetOptions){if(widgetOptions.extraKeys){return widgetOptions.extraKeys}const defaultKeys=["PI"];if(widgetOptions.answerForms==null){return defaultKeys}const uniqueExtraVariables={};const uniqueExtraConstants={};for(const answerForm of widgetOptions.answerForms){const maybeExpr=KAS.parse(answerForm.value,widgetOptions);if(maybeExpr.parsed){const expr=maybeExpr.expr;const isGreek=symbol=>symbol==="pi"||symbol==="theta";const toKey=symbol=>isGreek(symbol)?symbol.toUpperCase():symbol;const isKey=key=>KeypadKeys.includes(key);for(const variable of expr.getVars()){const maybeKey=toKey(variable);if(isKey(maybeKey)){uniqueExtraVariables[maybeKey]=true;}}for(const constant of expr.getConsts()){const maybeKey=toKey(constant);if(isKey(maybeKey)){uniqueExtraConstants[maybeKey]=true;}}}}const extraVariables=Object.keys(uniqueExtraVariables).sort();const extraConstants=Object.keys(uniqueExtraConstants).sort();const extraKeys=[...extraVariables,...extraConstants];if(!extraKeys.length){return defaultKeys}return extraKeys}
 
-function convert(f){return (rawValue,ctx)=>ctx.success(f(rawValue))}
+function convert(f){return (rawValue,ctx)=>ctx.success(f(rawValue,ctx))}
 
 const parseLegacyButtonSet=enumeration("basic","basic+div","trig","prealgebra","logarithms","basic relations","advanced relations","scientific");const parseLegacyButtonSets=defaulted(array(parseLegacyButtonSet),()=>["basic","trig","prealgebra","logarithms"]);
 
-function versionedWidgetOptions(latestMajorVersion,parseLatest){return new VersionedWidgetOptionsParserBuilder(latestMajorVersion,parseLatest,latest=>latest,(raw,ctx)=>ctx.failure("widget options with a known version number",raw))}class VersionedWidgetOptionsParserBuilder{withMigrationFrom(majorVersion,parseOldVersion,migrateToNextVersion){const parseOtherVersions=this.parser;const migrateToLatest=old=>this.migrateToLatest(migrateToNextVersion(old));return new VersionedWidgetOptionsParserBuilder(majorVersion,parseOldVersion,migrateToLatest,parseOtherVersions)}constructor(majorVersion,parseThisVersion,migrateToLatest,parseOtherVersions){this.migrateToLatest=migrateToLatest;this.parseOtherVersions=parseOtherVersions;const parseThisVersionAndMigrateToLatest=pipeParsers(parseThisVersion).then(convert(this.migrateToLatest)).parser;this.parser=(raw,ctx)=>{if(!isPlainObject(raw)){return ctx.failure("object",raw)}const versionParseResult=parseVersionedObject(raw,ctx);if(isFailure(versionParseResult)){return versionParseResult}if(versionParseResult.value.version.major!==majorVersion){return this.parseOtherVersions(raw,ctx)}return parseThisVersionAndMigrateToLatest(raw,ctx)};}}const parseVersionedObject=object({version:defaulted(object({major:number,minor:number}),()=>({major:0,minor:0}))});
+function versionedWidgetOptions(latestMajorVersion,parseLatest){return new VersionedWidgetOptionsParserBuilder(latestMajorVersion,parseLatest,latest=>latest,(raw,ctx)=>ctx.failure("widget options with a known version number",raw))}class VersionedWidgetOptionsParserBuilder{withMigrationFrom(majorVersion,parseOldVersion,migrateToNextVersion){const parseOtherVersions=this.parser;const migrateToLatest=(old,ctx)=>this.migrateToLatest(migrateToNextVersion(old,ctx),ctx);return new VersionedWidgetOptionsParserBuilder(majorVersion,parseOldVersion,migrateToLatest,parseOtherVersions)}constructor(majorVersion,parseThisVersion,migrateToLatest,parseOtherVersions){this.migrateToLatest=migrateToLatest;this.parseOtherVersions=parseOtherVersions;const parseThisVersionAndMigrateToLatest=pipeParsers(parseThisVersion).then(convert(this.migrateToLatest)).parser;this.parser=(raw,ctx)=>{if(!isPlainObject(raw)){return ctx.failure("object",raw)}const versionParseResult=parseVersionedObject(raw,ctx);if(isFailure(versionParseResult)){return versionParseResult}if(versionParseResult.value.version.major!==majorVersion){return this.parseOtherVersions(raw,ctx)}return parseThisVersionAndMigrateToLatest(raw,ctx)};}}const parseVersionedObject=object({version:defaulted(object({major:number,minor:number}),()=>({major:0,minor:0}))});
 
 const stringOrNumberOrNullOrUndefined=union(string).or(number).or(constant(null)).or(constant(undefined)).parser;function numberOrNullToString(v){return typeof v==="number"||v===null?String(v):v}const parsePossiblyInvalidAnswerForm=object({value:optional(string),form:defaulted(boolean,()=>false),simplify:defaulted(boolean,()=>false),considered:enumeration("correct","wrong","ungraded"),key:pipeParsers(stringOrNumberOrNullOrUndefined).then(convert(numberOrNullToString)).parser});function removeInvalidAnswerForms(possiblyInvalid){return possiblyInvalid.flatMap(answerForm=>{const{value}=answerForm;if(value!=null){return [{...answerForm,value}]}return []})}const parseAnswerForms=pipeParsers(defaulted(array(parsePossiblyInvalidAnswerForm),()=>[])).then(convert(removeInvalidAnswerForms)).parser;const version2$1=object({major:constant(2),minor:number});const parseExpressionWidgetV2=parseWidgetWithVersion(version2$1,constant("expression"),object({answerForms:parseAnswerForms,functions:array(string),times:boolean,visibleLabel:optional(string),ariaLabel:optional(string),buttonSets:parseLegacyButtonSets,buttonsVisible:optional(enumeration("always","never","focused")),extraKeys:optional(array(enumeration(...KeypadKeys)))}));const version1$1=object({major:constant(1),minor:number});const parseExpressionWidgetV1=parseWidgetWithVersion(version1$1,constant("expression"),object({answerForms:parseAnswerForms,functions:array(string),times:boolean,visibleLabel:optional(string),ariaLabel:optional(string),buttonSets:parseLegacyButtonSets,buttonsVisible:optional(enumeration("always","never","focused"))}));function migrateV1ToV2$1(widget){const{options}=widget;return {...widget,version:{major:2,minor:0},options:{times:options.times,buttonSets:options.buttonSets,functions:options.functions,buttonsVisible:options.buttonsVisible,visibleLabel:options.visibleLabel,ariaLabel:options.ariaLabel,answerForms:options.answerForms,extraKeys:deriveExtraKeys(options)??[]}}}const version0$1=optional(object({major:constant(0),minor:number}));const parseExpressionWidgetV0=parseWidgetWithVersion(version0$1,constant("expression"),object({functions:array(string),times:boolean,visibleLabel:optional(string),ariaLabel:optional(string),form:boolean,simplify:boolean,value:string,buttonSets:parseLegacyButtonSets,buttonsVisible:optional(enumeration("always","never","focused"))}));function migrateV0ToV1$1(widget){const{options}=widget;return {...widget,version:{major:1,minor:0},options:{times:options.times,buttonSets:options.buttonSets,functions:options.functions,buttonsVisible:options.buttonsVisible,visibleLabel:options.visibleLabel,ariaLabel:options.ariaLabel,answerForms:[{considered:"correct",form:options.form,simplify:options.simplify,value:options.value}]}}}const parseExpressionWidget=versionedWidgetOptions(2,parseExpressionWidgetV2).withMigrationFrom(1,parseExpressionWidgetV1,migrateV1ToV2$1).withMigrationFrom(0,parseExpressionWidgetV0,migrateV0ToV1$1).parser;
 
@@ -134,7 +134,7 @@
 
 function deriveNumCorrect(choices){return choices.filter(c=>c.correct).length}
 
-const parseWidgetsMapOrUndefined=defaulted(optional((rawVal,ctx)=>parseWidgetsMap(rawVal,ctx)),()=>undefined);function getDefaultOptions(){return {choices:[{content:""},{content:""},{content:""},{content:""}]}}const parseOnePerLine=defaulted(optional(boolean),()=>undefined);const parseNoneOfTheAbove=defaulted(optional(constant(false)),()=>undefined);const version3=optional(object({major:constant(3),minor:number}));const parseRadioWidgetV3=parseWidgetWithVersion(version3,constant("radio"),object({numCorrect:optional(number),choices:array(object({content:defaulted(string,()=>""),rationale:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean)})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean)}));const version2=optional(object({major:constant(2),minor:number}));const parseRadioWidgetV2=parseWidgetWithVersion(version2,constant("radio"),defaulted(object({numCorrect:optional(number),choices:array(object({content:defaulted(string,()=>""),clue:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),widgets:parseWidgetsMapOrUndefined})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean),onePerLine:parseOnePerLine,displayCount:optional(any),noneOfTheAbove:parseNoneOfTheAbove}),getDefaultOptions));const version1=optional(object({major:constant(1),minor:number}));const parseRadioWidgetV1=parseWidgetWithVersion(version1,constant("radio"),defaulted(object({choices:array(object({content:defaulted(string,()=>""),clue:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),widgets:parseWidgetsMapOrUndefined})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean),onePerLine:parseOnePerLine,displayCount:optional(any),noneOfTheAbove:parseNoneOfTheAbove}),getDefaultOptions));const version0=optional(object({major:constant(0),minor:number}));const parseRadioWidgetV0=parseWidgetWithVersion(version0,constant("radio"),defaulted(object({choices:array(object({content:defaulted(string,()=>""),clue:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),widgets:parseWidgetsMapOrUndefined})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean),onePerLine:parseOnePerLine,displayCount:optional(any),noneOfTheAbove:parseNoneOfTheAbove}),getDefaultOptions));function migrateV2toV3(widget){const{options}=widget;return {...widget,version:{major:3,minor:0},options:{numCorrect:options.numCorrect,hasNoneOfTheAbove:options.hasNoneOfTheAbove,countChoices:options.countChoices,randomize:options.randomize,multipleSelect:options.multipleSelect,deselectEnabled:options.deselectEnabled,choices:options.choices.map(choice=>({content:choice.content,rationale:choice.clue,correct:choice.correct,isNoneOfTheAbove:choice.isNoneOfTheAbove}))}}}function migrateV1ToV2(widget){const{options}=widget;return {...widget,version:{major:2,minor:0},options:{...options,numCorrect:deriveNumCorrect(options.choices)}}}function migrateV0ToV1(widget){const{options}=widget;const{noneOfTheAbove:_,...rest}=options;return {...widget,version:{major:1,minor:0},options:{...rest,hasNoneOfTheAbove:false,noneOfTheAbove:undefined}}}const parseRadioWidget=versionedWidgetOptions(3,parseRadioWidgetV3).withMigrationFrom(2,parseRadioWidgetV2,migrateV2toV3).withMigrationFrom(1,parseRadioWidgetV1,migrateV1ToV2).withMigrationFrom(0,parseRadioWidgetV0,migrateV0ToV1).parser;
+const parseWidgetsMapOrUndefined=defaulted(optional((rawVal,ctx)=>parseWidgetsMap(rawVal,ctx)),()=>undefined);function getDefaultOptions(){return {choices:[{content:""},{content:""},{content:""},{content:""}]}}const parseOnePerLine=defaulted(optional(boolean),()=>undefined);const parseNoneOfTheAbove=defaulted(optional(constant(false)),()=>undefined);const version3=optional(object({major:constant(3),minor:number}));const parseRadioWidgetV3=parseWidgetWithVersion(version3,constant("radio"),object({numCorrect:optional(number),choices:array(object({content:defaulted(string,()=>""),rationale:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),id:defaulted(string,(_,ctx)=>ctx.getPath().join("."))})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean)}));const version2=optional(object({major:constant(2),minor:number}));const parseRadioWidgetV2=parseWidgetWithVersion(version2,constant("radio"),defaulted(object({numCorrect:optional(number),choices:array(object({content:defaulted(string,()=>""),clue:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),widgets:parseWidgetsMapOrUndefined})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean),onePerLine:parseOnePerLine,displayCount:optional(any),noneOfTheAbove:parseNoneOfTheAbove}),getDefaultOptions));const version1=optional(object({major:constant(1),minor:number}));const parseRadioWidgetV1=parseWidgetWithVersion(version1,constant("radio"),defaulted(object({choices:array(object({content:defaulted(string,()=>""),clue:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),widgets:parseWidgetsMapOrUndefined})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean),onePerLine:parseOnePerLine,displayCount:optional(any),noneOfTheAbove:parseNoneOfTheAbove}),getDefaultOptions));const version0=optional(object({major:constant(0),minor:number}));const parseRadioWidgetV0=parseWidgetWithVersion(version0,constant("radio"),defaulted(object({choices:array(object({content:defaulted(string,()=>""),clue:optional(string),correct:optional(boolean),isNoneOfTheAbove:optional(boolean),widgets:parseWidgetsMapOrUndefined})),hasNoneOfTheAbove:optional(boolean),countChoices:optional(boolean),randomize:optional(boolean),multipleSelect:optional(boolean),deselectEnabled:optional(boolean),onePerLine:parseOnePerLine,displayCount:optional(any),noneOfTheAbove:parseNoneOfTheAbove}),getDefaultOptions));function migrateV2toV3(widget,ctx){const{options}=widget;return {...widget,version:{major:3,minor:0},options:{numCorrect:options.numCorrect,hasNoneOfTheAbove:options.hasNoneOfTheAbove,countChoices:options.countChoices,randomize:options.randomize,multipleSelect:options.multipleSelect,deselectEnabled:options.deselectEnabled,choices:options.choices.map((choice,i)=>({content:choice.content,rationale:choice.clue,correct:choice.correct,isNoneOfTheAbove:choice.isNoneOfTheAbove,id:[...ctx.getPath(),"options","choices",i].join(".")}))}}}function migrateV1ToV2(widget){const{options}=widget;return {...widget,version:{major:2,minor:0},options:{...options,numCorrect:deriveNumCorrect(options.choices)}}}function migrateV0ToV1(widget){const{options}=widget;const{noneOfTheAbove:_,...rest}=options;return {...widget,version:{major:1,minor:0},options:{...rest,hasNoneOfTheAbove:false,noneOfTheAbove:undefined}}}const parseRadioWidget=versionedWidgetOptions(3,parseRadioWidgetV3).withMigrationFrom(2,parseRadioWidgetV2,migrateV2toV3).withMigrationFrom(1,parseRadioWidgetV1,migrateV1ToV2).withMigrationFrom(0,parseRadioWidgetV0,migrateV0ToV1).parser;
 
 const parseSorterWidget=parseWidget(constant("sorter"),object({correct:array(string),padding:boolean,layout:enumeration("horizontal","vertical")}));
 

@benchristel benchristel force-pushed the benc/assign-radio-ids-by-location branch from 33bb9f0 to 5233d5c Compare August 6, 2025 20:27
Copy link
Contributor

github-actions bot commented Aug 6, 2025

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (c9db91a) and published it to npm. You
can install it using the tag PR2664.

Example:

pnpm add @khanacademy/perseus@PR2664

If you are working in Khan Academy's frontend, you can run the below command.

./dev/tools/bump_perseus_version.ts -t PR2664

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR2664

Copy link
Member Author

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

@anakaren-rojas this commit has the relevant change: 7daa1ae

content: choice.content,
rationale: choice.clue,
correct: choice.correct,
isNoneOfTheAbove: choice.isNoneOfTheAbove,
id: [...ctx.getPath(), "options", "choices", i].join("."),
Copy link
Contributor

Choose a reason for hiding this comment

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

oh interesting, I tried getting access to the path here but got stuck on the implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
item-splitting-change olc-5.0.7d71e schema-change Attached to PRs when we detect Perseus Schema changes in it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants