Skip to content

fix: move define-properties to preferred manifest#755

Closed
morgan-coded wants to merge 4 commits into
e18e:mainfrom
morgan-coded:fix/define-properties-not-polyfill
Closed

fix: move define-properties to preferred manifest#755
morgan-coded wants to merge 4 commits into
e18e:mainfrom
morgan-coded:fix/define-properties-not-polyfill

Conversation

@morgan-coded

@morgan-coded morgan-coded commented Jun 26, 2026

Copy link
Copy Markdown

🔗 Linked issue

es-tooling/eslint-plugin-depend#35

📚 Description

Moves define-properties from the native manifest to the preferred manifest because it is not always a direct native drop-in for Object.defineProperties.

This also adds a docs page for the preferred entry, with the distinction that Object.defineProperties is enough when callers only need to define descriptors, while define-properties has extra behavior around existing properties and predicate-controlled overrides.

Checks:
pnpm lint

Copilot AI review requested due to automatic review settings June 26, 2026 05:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gameroman

Copy link
Copy Markdown
Contributor

A package goes to preferred manifest if it has a documented replacements so you would need to add a .md file explaining the replacement,
for example how it's done for buffer-equal-constant-time

Otherwise it should stay in the native manifest since it is still replaceable by native functionality

@gameroman

Copy link
Copy Markdown
Contributor

Also please use the PR template

@43081j

43081j commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

given that you can conditionally define properties natively, too, can you explain why someone should use a package to do it instead?

e.g.

Object.defineProperties(obj, {
  // Always defined
  id: {
    value: 303
  },

  // Conditionally defined
  ...(flags.enableFoo && {
    foo: {
      value: "foo value"
    },
  }),

  ...(flags.enableBar && {
    bar: {
      value: "bar value"
    },
  }),
});

@gameroman

Copy link
Copy Markdown
Contributor

@43081j I might be wrong but this pr honestly looks like it was opened by a bot

@morgan-coded

Copy link
Copy Markdown
Author

I updated this to follow the preferred-entry convention: define-properties now has a docs page and the manifest entry links to it.

I’m not trying to argue that Object.defineProperties can’t cover conditional cases with normal JS. The issue is that the current native warning feels too absolute for this package, since define-properties also has behavior around existing properties and predicate-controlled overrides. The docs now frame Object.defineProperties as the right replacement when that extra behavior is not needed.

Happy to switch direction or close this if you’d rather keep the native classification.

## `Object.defineProperties` (native)

[`Object.defineProperties`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperties) can define multiple properties on an object without an extra dependency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to show a diff for migration like in https://github.com/e18e/module-replacements/blob/main/docs/modules/clipboardy.md for example

@morgan-coded

Copy link
Copy Markdown
Author

Updated the example to use the migration diff format. pnpm lint is still passing.

Comment on lines +31 to +45

```ts
const descriptors = {
enabled: {
configurable: true,
enumerable: false,
value: true,
writable: false
}
}

if (!Object.hasOwn(obj, 'enabled') || shouldOverrideEnabled()) {
Object.defineProperties(obj, descriptors)
}
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a diff for this too please?

@morgan-coded

Copy link
Copy Markdown
Author

Updated the second example to use the migration diff format too. pnpm lint is still passing.

@43081j

43081j commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

I’m not trying to argue that Object.defineProperties can’t cover conditional cases with normal JS. The issue is that the current native warning feels too absolute for this package, since define-properties also has behavior around existing properties and predicate-controlled overrides. The docs now frame Object.defineProperties as the right replacement when that extra behavior is not needed.

If you understand that Object.defineProperties can cover conditional cases, what is the extra behaviour we can't do natively that justifies pulling a package in?

You say also has behavior around existing properties and predicate-controlled overrides. Given we know you can do conditional properties natively, that just leaves behavior around existing properties, yes? What is that behaviour that can't be done natively?

@morgan-coded

Copy link
Copy Markdown
Author

Thanks for pushing on the distinction here.

I rechecked define-properties, and I agree this is more convenience/ergonomics than a real native capability gap. The existing behavior around skipping existing keys and predicate-controlled overrides is useful for shim-style code, but it can still be expressed with Object.defineProperties and normal JS conditionals.

So I think native is the better classification after all. I’m going to close this to avoid churn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants