Clay: Remove non-default export from components

[matuzalemsteles]

See the discussions at #1453 for context.

This is a list of components that we need to add warnings when someone is using non-default export from components.

• [ ] clay-alert
• [ ] clay-autocomplete
• [ ] clay-badge
• [ ] clay-button
• [ ] clay-card-grid
• [ ] clay-charts
• [ ] clay-checkbox
• [ ] clay-collapse
• [ ] clay-component
• [ ] clay-data-provider
• [ ] clay-dataset-display
• [ ] clay-icon
• [ ] clay-label
• [ ] clay-link
• [ ] clay-list
• [ ] clay-loading-indicator
• [ ] clay-modal
• [ ] clay-multi-select
• [ ] clay-navigation-bar
• [ ] clay-pagination-bar
• [ ] clay-pagination
• [ ] clay-portal
• [ ] clay-progress-bar
• [ ] clay-radio
• [ ] clay-select
• [ ] clay-sticker
• [ ] clay-table
• [ ] clay-tooltip

[jbalsas]

Hey, since we're moving on with 3.x... should we even try to take care of this for the 2.x branch? Shouldn't we simply apply this for the 3.x branch directly?

[matuzalemsteles]

hey @jbalsas, I do not know if I understand. But the idea is to add the warnings to 2.x which are components of Metal.js and remove in later versions of 2.x. And 3.x are we already removing with the React components or should we leave 2.x as it is and move on only with 3.x?

[jbalsas]

That's what I was thinking... we should never remove the default export sin the 2.x branch. As moving to 3.x is already a clear breaking change and will be signaled both by the version and maybe the package scope... I'm trying to think if warning about this in 2.x is even worth it.

So yeah, what I'm trying to say is, we could just leave 2.x as is and properly apply the new pattern for 3.x only. 🤷‍♀️

[matuzalemsteles]

ok, makes sense. hey @wincent, do you have any opinion on this?

[wincent]

I think there's not much point in removing the existing non-defaults in the 2.x. (Removing from the generator is probably fine though, and we already did that in #1453). But it may be worth adding the deprecation warnings when they're used, which is what I think you created this PR for.

If we weren't already introducing a massive change (and a breaking one) in moving to React in 3.0, then I would say that deprecate-with-warning-then-remove would be the right thing to do. But as this change is happening at the same time, then maybe we don't need the warnings.

[jbalsas]

So, how would we go about adding warnings for non-default export usage?

If we can achieve that, I'm all for it... it should be easier to spot usages, although they will likely go away on its own as we migrate code to 3.x so it might still be pointless.

In any case, let's at least make the thought exercise, if it's cheap enough we can add it and we'll know how to do similar things in the future :)

[wincent]

Untested, but I was thinking something like this:

class FooComponent {
  // class body...
}

let FooComponent_ = FooComponent;
if (__DEV__) {
  FooComponent_ = class extends FooComponent  {
    constructor(...args) {
      const instance = super(...args);
      console.warn('Deprecation warning: ...');
      return instance;
    }
  };
}

export {FooComponent_ as FooComponent};
export default FooComponent;

Obviously if this works, we could abstract it in some way:

import deprecatedExport from '../utils/deprecationUtils';

class FooComponent {}

const FooComponent_ = deprecatedExport(FooComponent);

export {FooComponent_ as FooComponent};
export default FooComponent;

The naming could probably be improved but you get the idea. That's for component classes; we could have similar patterns for function exports that we want to deprecate.

[matuzalemsteles]

hey guys, sorry for the confusion I had thought we would continue to remove non-default exports from Clay 2.x. I think the warnings would really be important to be placed...

[jbalsas]

No worries, @matuzalemsteles!

I'd give @wincent suggestion a try to see how it looks!

[matuzalemsteles]

I will look at this later to test and if successful, I will work in a PR to send.

[wincent]

One thing I forgot to add is that I think we should only emit one warning per component per run. So in the example of the constructor, we don't really want to fire on every single instance. If you end up abstracting this in a deprecatedExport helper, you'd want a WeakSet (or something) that you can use to track whether a particular entity has already had a warning issued for it.

[jbalsas]

Doing some spring-cleaning... closing all issues labeled as 2.x that are not type:bug since 2.x is in maintenance and we won't be proactively be taking these ones.

I think we can discard this for 2.x. @matuzalemsteles, @wincent, feel free to add to the conversation and/or reopen if you think this is still relevant.

Thanks!