feat: Environment locks #26

akazemier-godaddy · 2025-11-03T18:16:28Z

Introducing environment/boundary locks to flag when data-override should be introduced.

Checklist

My code follows the project's code style (biome)
I have added/updated tests that prove my fix is effective or that my feature works
I have added/updated documentation (README, Storybook stories, etc.)
All new and existing tests pass locally
I have added a changeset (run npm run changeset if this affects packages)
I have reviewed my own code and commented any complex areas

changeset-bot · 2025-11-03T18:16:32Z

🦋 Changeset detected

Latest commit: 2054f8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 24 packages

Name	Type
@bento/environment	Major
@bento/use-props	Major
@bento/slots	Major
@bento/box	Major
@bento/use-svg-sprite	Patch
@bento/button	Patch
@bento/checkbox	Patch
@bento/container	Patch
@bento/dismiss	Patch
@bento/divider	Patch
@bento/field-error	Patch
@bento/focus-lock	Patch
@bento/heading	Patch
@bento/icon	Patch
@bento/illustration	Patch
@bento/input	Patch
@bento/listbox	Patch
@bento/overlay	Patch
@bento/portal	Patch
@bento/pressable	Patch
@bento/radio	Patch
@bento/scroll-lock	Patch
@bento/text	Patch
@bento/visually-hidden	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copilot

Pull Request Overview

This PR introduces "environment locks" as a boundary mechanism to control when data-override attributes should be introduced in the Bento component system. The change fundamentally shifts the override detection system from always flagging modifications to only flagging them when they cross established lock boundaries, enabling design systems to distinguish between internal composition and consumer customizations.

Removes the @bento/internal-props package entirely and simplifies useProps to work directly with props
Introduces lock boundaries via Environment component's lock prop with generation tracking
Updates override detection to respect lock boundaries when determining whether to add data-override attributes

Reviewed Changes

Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`packages/use-props/src/index.ts`	Simplified to remove internal-props dependency and work directly with props
`packages/slots/src/slots.tsx`	Added generation tracking for slot assignments to support lock boundaries
`packages/slots/src/override.ts`	Updated override detection logic to respect lock boundaries and generations
`packages/environment/src/index.tsx`	Added `lock` prop to create boundaries and increment generation counters
`packages/box/src/index.ts`	Added lock-related properties to context interfaces
`packages/internal-props/*`	Entire package removed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-04T15:10:55Z

packages/slots/src/override.ts

+    if (!isLocked || slotGeneration < currentLockGeneration) {
+      causes.push('className');
+    }


[nitpick] The logic for determining when to flag className could be more explicit. Consider extracting this condition into a helper function like shouldFlagModification(isLocked, slotGeneration, currentLockGeneration) to improve readability and make the lock boundary logic more reusable."

Copilot · 2025-11-04T15:10:56Z

packages/slots/src/override.ts

+      if (!isLocked || slotGeneration < currentLockGeneration) {
+        causes.push('style');
+      }


[nitpick] This duplicates the same lock boundary condition as the className check above. Consider extracting this logic into a shared helper function to avoid code duplication."

packages/slots/src/slots.tsx

akazemier-godaddy · 2025-11-04T15:49:15Z

packages/internal-props/CHANGELOG.md

I'm guessing there's no deprecation feature in changesets?

ddamato-godaddy · 2025-11-06T16:38:56Z

packages/environment/src/index.tsx

+        ctx.env.locked = true;
+        ctx.env.lockGeneration = ctx.env.lockGeneration + 1;


Is there a benefit to having both locked and lockGeneration? Wondering if we'd be fine with checking that isNaN(ctx.env.lockGeneration) or ctx.env.lockGeneration < 1?

packages/use-props/src/index.ts

rmarkins-godaddy · 2025-11-06T17:10:09Z

packages/environment/test/examples.browser.test.tsx

+      const result = container.innerHTML;
+
+      // Should have NO data-override attributes since all slots are internal composition
+      assume(result).equals(


This is ridiculously huge. Could we not just use a snapshot here instead of doing this? https://vitest.dev/guide/snapshot.html#snapshot

Agreed, but also that is how the existing tests are also asserting structure.

So while I would agree that snapshots is a better option, this really is something that should be applied to all tests to set this as a standard going forward, and should be done as a separate, follow-up effort, instead of as part of this PR.

packages/environment/test/examples.browser.test.tsx

packages/slots/vitest.config.ts

kawikabader · 2025-11-06T17:24:21Z

packages/slots/src/override.ts

+
+  // Only flag className if environment is NOT locked, or if slot is from earlier generation
+  if ('className' in props && !causes.includes('className')) {
+    if (!isLocked || slotGeneration < currentLockGeneration) {


I noticed the lock boundary condition !isLocked || slotGeneration < currentLockGeneration appears in a couple places. Would it be worth extracting this into a small helper function?

kawikabader · 2025-11-06T17:27:07Z

packages/slots/src/slots.tsx

+        // If newSlot is from an earlier generation (consumer slot), update the tag
+        // We assume parent slots (assignedSlot) are from consumer, so keep their generation


Sorry, slightly confused by this comment. It seems like we're preserving the parent's generation (by not overwriting it), rather than updating the tag? Just want to make sure I understand.

kawikabader · 2025-11-06T17:31:12Z

packages/slots/src/override.ts

+  if (slot && isLocked && slotGeneration < currentLockGeneration) {
+    // Any slot modification from an earlier generation should be flagged
+    if (!causes.includes('slot')) {
+      causes.push('slot');
+    }
+    // Also add specific triggers if present
+    Object.keys(slot).forEach(function forEach(name) {
+      if (triggers.includes(name) && !causes.includes(name)) {
+        causes.push(name);
+      }
+    });


Quick question about the design here: looks like we're flagging both 'slot' AND the specific triggers (className/style), which produces something like data-override="style slot". Is the granularity needed for debugging or tooling purposes? Just curious if data-override="slot" alone would be sufficient, or if having the specifics is valuable.

If the detailed triggers are intentional (which makes sense for debugging!), it might be worth adding a quick comment explaining the reasoning for future maintainers maybe?

Great question, there's an important distinction here, class and style are visual overides while a slot can also be a functional override. E.g. you can choose to break/improve/change interaction patterns by overriding a onPress or onClick to a container. Visually, nothing would break, but functionally, it has a completely different impact.

Then if we want to go really deeply here, slots are not the only way to make modifications to components, you could take a component that ends up in your Design System and apply your own className to it, that would also trigger a data-override, but it's not caused by a slot.

But in the end, this detailed differentiation is there purely for a debugging point of view so when you inspect a component that doesn't function according to your pre-defined specifications.

If we feel that this is too verbose, we could also decide to just change the <Environment> api, and just have lock accept an array of triggers. e.g. <Environment lock=['slot'] />

Thanks for the clarification. The distinction makes sense, and the granularity would for sure help debugging.

Given this, should we consider adding a brief comment in the code explaining why both 'slot' and the specific triggers are included.

Regarding <Environment lock={['slot']} />, would that allow consumers to opt into less granular override tracking if they prefer? Or would it change the behavior entirely?

Given this, should we consider adding a brief comment in the code explaining why both 'slot' and the specific triggers are included.

Yes, i'll update the comments 👍

Regarding <Environment lock={['slot']} />, would that allow consumers to opt into less granular override tracking if they prefer? Or would it change the behavior entirely?

The Environment and locking would be something that we would apply as part of the components that we offer in our Design System. So I don't know how much sense it makes to add, but from an OS perspective, having the customization could be nice, just not sure if it's worth the squeeze.

- resolved conflicts by keeping internal-props package deleted - updated use-props package.json to remove internal-props dependency - regenerated package-lock.json

Resolved conflicts by: - Removed @bento/internal-props dependency (kept in environment-locks) - Added @react-aria/utils for ref merging - Integrated ref handling logic from main without internal-props - Regenerated package-lock.json

…ironment-locks * 'environment-locks' of github.com:godaddy/bento:

- Convert use-props, slots, and environment browser tests to use toMatchSnapshot() - Add withLock HOC for wrapping components with locked Environment - Fix use-props node tests to use locked environment when testing data-override - Add @bento/environment and @bento/use-props as devDependencies for slots tests

- Convert portal tests to use vitest snapshots - Fix TypeScript errors in box context tests (add locked, lockGeneration, slotGenerations) - Update knip config to ignore .ts test files for circular dependency detection - Remove circular dependency from slots package.json

Environment lock={true} can be used directly instead.

- Add tests for slot generation tagging on first/nested lock (100% coverage) - Fix TypeScript errors in lock-no-override example (cast components to accept children)

3rd-Eden added 2 commits November 3, 2025 18:32

major: env locking

9a1de8f

doc: improve docs

ed3a184

3rd-Eden added 5 commits November 3, 2025 21:33

fix: update packages with removal of internal props

d1dff30

fix: use env and use-props as part of tests

c95d854

fix: corrected assertion now that we properly merge slots

18135e6

fix: clean up references and dead branches

2221d3d

test: clean up tests

1431ebb

akazemier-godaddy marked this pull request as ready for review November 4, 2025 14:57

akazemier-godaddy requested a review from a team as a code owner November 4, 2025 14:57

3rd-Eden added 2 commits November 4, 2025 16:00

major: effectively breaking how data-override worked before

ed49047

fix: 90%, im sad about it

8981a44

akazemier-godaddy requested a review from Copilot November 4, 2025 15:09

Copilot AI reviewed Nov 4, 2025

View reviewed changes

3rd-Eden and others added 2 commits November 4, 2025 16:28

lint: linting

8757552

Merge branch 'main' into environment-locks

85a0f6b

akazemier-godaddy commented Nov 4, 2025

View reviewed changes

packages/internal-props/CHANGELOG.md

Copy link

Contributor Author

akazemier-godaddy Nov 4, 2025

Choose a reason for hiding this comment

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

I'm guessing there's no deprecation feature in changesets?

ddamato-godaddy reviewed Nov 6, 2025

View reviewed changes

packages/use-props/src/index.ts Show resolved Hide resolved

rmarkins-godaddy reviewed Nov 6, 2025

View reviewed changes

kawikabader reviewed Nov 6, 2025

View reviewed changes

3rd-Eden added 3 commits November 10, 2025 14:26

chore: merge origin/main into environment-locks

3d8c25e

- resolved conflicts by keeping internal-props package deleted - updated use-props package.json to remove internal-props dependency - regenerated package-lock.json

fix: it should only trigger on locked

03cf185

fix: update env

886759c

akazemier-godaddy changed the title ~~Environment locks~~ feat: Environment locks Nov 11, 2025

3rd-Eden and others added 3 commits November 14, 2025 17:45

fix: data-override should be disabled by default

2f43a67

fix: update tests as data-override is no longer triggering by default

c874953

Merge branch 'main' into environment-locks

fbce5ce

Merge branch 'main' into environment-locks

46cf74d

jsmith9-godaddy approved these changes Nov 19, 2025

View reviewed changes

akazemier-godaddy mentioned this pull request Nov 25, 2025

feat: Input primitive #54

Merged

6 tasks

ddamato-godaddy approved these changes Nov 26, 2025

View reviewed changes

3rd-Eden added 5 commits November 27, 2025 17:00

chore: merge main into environment-locks

3810c4b

Resolved conflicts by: - Removed @bento/internal-props dependency (kept in environment-locks) - Added @react-aria/utils for ref merging - Integrated ref handling logic from main without internal-props - Regenerated package-lock.json

Merge branch 'environment-locks' of github.com:godaddy/bento into env…

558dcda

…ironment-locks * 'environment-locks' of github.com:godaddy/bento:

chore: merge main - keep data-override disabled by default

cbe76cc

docs: clean up

5fd7f75

akazemier-godaddy force-pushed the environment-locks branch from d750bfb to 5fd7f75 Compare December 19, 2025 00:17

3rd-Eden added 3 commits December 19, 2025 01:27

chore: remove withLock HOC - not needed

457d22b

Environment lock={true} can be used directly instead.

fix: add coverage tests and fix TypeScript errors

2054f8c

- Add tests for slot generation tagging on first/nested lock (100% coverage) - Fix TypeScript errors in lock-no-override example (cast components to accept children)

		ctx.env.locked = true;
		ctx.env.lockGeneration = ctx.env.lockGeneration + 1;

		// If newSlot is from an earlier generation (consumer slot), update the tag
		// We assume parent slots (assignedSlot) are from consumer, so keep their generation

feat: Environment locks #26

Are you sure you want to change the base?

feat: Environment locks #26

Uh oh!

Conversation

akazemier-godaddy commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

changeset-bot bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

akazemier-godaddy commented Nov 3, 2025 •

edited

Loading

changeset-bot bot commented Nov 3, 2025 •

edited

Loading