Skip to content
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

Deprecated defaultValue.EMPTY_OBJECT and created EmptyObject standalone constant #12238

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

anpaulan
Copy link

@anpaulan anpaulan commented Oct 7, 2024

Description

  • Fixed missing JSDOC for EMPTY_OBJECT
  • Deprecated EMPTY_OBJECT property in defaultValue
  • Created standalone EmptyObject constant (via EmptyObject.js) per recommendation

Collaborated with @hotpocket

Issue number and link

Fixes Issue #11326

Testing plan

Tried to run this test in defaultValueSpec.js, but console.warn was not called: test did not run as expected.

it("Calls console.warn", function () {
    spyOn(console, 'warn');
    defaultValue.EMPTY_OBJECT
    expect(console.warn).toHaveBeenCalled();
  });

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Oct 7, 2024

Thank you for the pull request, @anpaulan!

✅ We can confirm we have a CLA on file for you.

@anpaulan anpaulan changed the title Deprecated defaultValue.EMPTY_OBJECT and created EmptyObject standalone constant #11326 Deprecated defaultValue.EMPTY_OBJECT and created EmptyObject standalone constant Oct 7, 2024
@jjspace
Copy link
Contributor

jjspace commented Oct 7, 2024

Thanks for the pr @anpaulan. However I don't think we want to make this change currently. I didn't remember there were 2 issues open related to this.

PR #12207 for issue #11674 is removing the defaultValue function and should be changing how we handle the EMPTY_OBJECT already. I don't think we want both of these changes. In that PR/issue we had settled on leaving it in the namespace of defaultValue. I believe removing the function should also inherently fix the typescript issue too but we can be sure to check that in that PR.

Maybe you could work with @dave-b-b to combine the changes?

CC @ggetz

@ggetz
Copy link
Contributor

ggetz commented Oct 9, 2024

Good catch on the conflict here @jjspace.

To sum up the result of the discussion around #11674, it sounds like we'd want to:

  • Deprecate defaultValue (the function)
  • Keep defaultValue the namespace

Correct?

If that's the case, we should be able to do something like the following and still resolve the typescript error. If it becomes a namespace, defaultValue should be renamed DefaultValue.

/**
 * Utilities helpful for setting a default value for a parameter.
 *
 * @namespace DefaultValue
 */
const DefaultValue = {};

/**
 * A frozen empty object that can be used as the default value for options passed as
 * an object literal.
 * @type {object}
 * @memberof DefaultValue
 */
DefaultValue.EMPTY_OBJECT = Object.freeze({});

export default DefaultValue;

In that case, defaultValue and defaultValue.EMPTY_OBJECT will still need to be deprecated.

@anpaulan
Copy link
Author

Good catch on the conflict here @jjspace.

To sum up the result of the discussion around #11674, it sounds like we'd want to:

  • Deprecate defaultValue (the function)
  • Keep defaultValue the namespace

Correct?

If that's the case, we should be able to do something like the following and still resolve the typescript error. If it becomes a namespace, defaultValue should be renamed DefaultValue.

/**
 * Utilities helpful for setting a default value for a parameter.
 *
 * @namespace DefaultValue
 */
const DefaultValue = {};

/**
 * A frozen empty object that can be used as the default value for options passed as
 * an object literal.
 * @type {object}
 * @memberof DefaultValue
 */
DefaultValue.EMPTY_OBJECT = Object.freeze({});

export default DefaultValue;

In that case, defaultValue and defaultValue.EMPTY_OBJECT will still need to be deprecated.

Hi! Thank you for sending this over.

Good catch on the conflict here @jjspace.

To sum up the result of the discussion around #11674, it sounds like we'd want to:

  • Deprecate defaultValue (the function)
  • Keep defaultValue the namespace

Correct?

If that's the case, we should be able to do something like the following and still resolve the typescript error. If it becomes a namespace, defaultValue should be renamed DefaultValue.

/**
 * Utilities helpful for setting a default value for a parameter.
 *
 * @namespace DefaultValue
 */
const DefaultValue = {};

/**
 * A frozen empty object that can be used as the default value for options passed as
 * an object literal.
 * @type {object}
 * @memberof DefaultValue
 */
DefaultValue.EMPTY_OBJECT = Object.freeze({});

export default DefaultValue;

In that case, defaultValue and defaultValue.EMPTY_OBJECT will still need to be deprecated.

Thank you for the guidance! I was going through this PR as well as PR #12207 and I think you are right. I can go ahead and deprecate defaultValue and update the name space. But when I do so, I would need to work with @dave-b-b to figure out what would be used in place of defaultValue? My understanding is when I deprecate defaultValue, we should then redirect folks to the replacement function.

@ggetz
Copy link
Contributor

ggetz commented Oct 14, 2024

My understanding is when I deprecate defaultValue, we should then redirect folks to the replacement function.

Yes, this correct.

I think it's be worth asking @dave-b-b what the current timeline on #12207 is, and coordinate changes as needed. I'll add a message to that PR.

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.

3 participants