Allow setting builds as args rather than parameters

[tmeasday]

Trying this out - is this what you had in mind @yannbf? It seems like the types don't like it though @kasperpeulen (see the @ts-expect-error, not quite sure why. I can set args.AddonVisualTestsBuild in the meta but not in the story.

It does work though.

📦 Published PR as canary version: 0.0.64--canary.90.6a458fa.0

✨ Test out this PR locally via:

npm install @chromaui/[email protected]
# or 
yarn add @chromaui/[email protected]

[tmeasday]

For some reason it errors if I set this arg on the Story but not on the Meta.

[shilman]

Does it make more sense to make more sense for the arg to be the input to constructResult if we want the user to be able to edit this in the controls panel?

[tmeasday]

How would that work @shilman? constructResult is specific to this query...

[kasperpeulen]

Ah, yes, if you use typeof meta, it only looks at the component type, and it can really do anything with the satisfies part.

You will have to do something as:

type StoryArgs = Parameters<typeof VisualTests>[0] & {
    AddonVisualTestsBuild: ResultOf<typeof QueryBuild>;
}

const meta = { } satisfies Meta<StoryArgs>;

type Story = StoryObj<StoryArgs>

[tmeasday]

@kasperpeulen but am I correct in saying that would mean we would lose any args that were defined in meta (in terms of being required?)

[kasperpeulen]

That is right, you have to set them optional yourself in StoryObj.

I have thought about this scenario, but I think there is no way, as the satisfies constraint has no effect on the inferred return type of meta.

Factories would solve this problem 🤓

[tmeasday]

I figured you'd say that :)

[tmeasday]

This worked OK, FTR:

Only problem was a couple of stories that had a render() function, the args were typed wrongly :/

[shilman]

Cool!!

[kasperpeulen]

Really nice to see that args can be used for together with msw, would make it much more powerful!

[tmeasday]

Closing this in favour of a similar change in #124

Note in that PR I nested the args under $graphql:

addon-visual-tests/src/screens/VisualTests/VisualTests.stories.tsx

Lines 281 to 298 in fcaf963

	args: {
	$graphql: {
	AddonVisualTestsBuild: {
	lastBuildOnBranch: withTests(pendingBuild, pendingTests),
	},
	},
	localBuildProgress: {
	...EmptyBranchLocalBuildCapturing.args.localBuildProgress,
	buildProgressPercentage: 90,
	stepProgress: {
	...EmptyBranchLocalBuildCapturing.args.localBuildProgress.stepProgress,
	snapshot: {
	...EmptyBranchLocalBuildCapturing.args.localBuildProgress.stepProgress.snapshot,
	numerator: 310,
	},
	},
	},
	},

I also add a "mapping" concept:

addon-visual-tests/src/screens/VisualTests/VisualTests.stories.tsx

Lines 136 to 141 in fcaf963

	argTypes: {
	addNotification: { type: "function", target: "manager-api" },
	$graphql: {
	AddonVisualTestsBuild: { map: mapQuery },
	},
	},

addon-visual-tests/src/screens/VisualTests/VisualTests.stories.tsx

Lines 101 to 125 in fcaf963

	function mapQuery(
	{ lastBuildOnBranch, userCanReview = true, ...input }: QueryInput,
	{ selectedBuildId }: VariablesOf<typeof QueryBuild>
	) {
	const possibleSelectedBuilds =
	("selectedBuild" in input && input.selectedBuild && [input.selectedBuild]) ||
	("selectedBuilds" in input && input.selectedBuilds) ||
	[];
	if (lastBuildOnBranch) possibleSelectedBuilds.push(lastBuildOnBranch);
	const selectedBuild = possibleSelectedBuilds.find((b) => b.id === selectedBuildId);
	console.log({ possibleSelectedBuilds, lastBuildOnBranch, selectedBuild, selectedBuildId });
	return {
	project: {
	name: "acme",
	lastBuildOnBranch: lastBuildOnBranch || selectedBuild,
	},
	selectedBuild,
	viewer: {
	projectMembership: {
	userCanReview,
	},
	},
	};
	}

It works pretty well. @shilman I think nesting under $graphql makes it a little more intuitive to read, but a bit more awkward to write--ie:

export const StoryAddedNotInBuild = {
  args: {
    $graphql: {
      AddonVisualTestsBuild: {
        selectedBuild: withTests({ ...pendingBuild }, []),
      },
    },
  },
}

// vs 

export const StoryAddedNotInBuild = {
  args: {
    AddonVisualTestsBuild: {
      selectedBuild: withTests({ ...pendingBuild }, []),
    },
  },
}

On the one hand the extra level of nesting is just more work all the time, on the other hand it makes it much more clear what this arg that has nothing to do with the component's props is doing.

[shilman]

Haven't thought this through, but I wonder if we could just hoist $graphql and make $x the convention for targeted args, reducing one level of hierarchy?

[tmeasday]

@shilman we could but I guess I was saying the $graphql is actually useful because at a glance it distinguishes the target of the args when you read the story and don't have all the context.

If it was $AddonVisualTestsBuild it would still be clear it's not a prop, but not where it's going.

Maybe that's a good tradeoff though? I guess in any given project you are probably only going to have one or two arg "targets".

If we did it that way, we could even just make the $- thing a convention, given you'll have to define somewhere that $AddonVisualTestsBuild targets graphql.

[shilman]

Sorry I meant the other way. Pull $graphql to be a peer of args.

[tmeasday]

So a new annotation?

[yannbf]

I think it makes sense to have them under args:

1. People already connect args to the controls table, or addons that provide a controls-like experience
2. You still maintain a scope, rather than having a freeform property in the story top annotation level
3. The $ prefix (maybe it should be $$) helps distinguish what is not a prop, the only framework that prepends certain properties with $ is angular, for observables

[shilman]

Yeah a new class of annotation. Where we could have $graphql, $mock, $<yourTargetHere>.

I don't love it, but I'm bringing it up because we always try to be careful about scoping things correctly and then we generate these deeply nested structures that are not that ergonomic, and then we end up hoisting them later. But maybe we should consider just hoisting from the beginning under the assumption that targeted args will be a big deal and people will be using them a lot.