eBay / ebayui-core

Collection of Marko widgets; considered to be the core building blocks for all eBay components, pages & apps
https://ebay.github.io/ebayui-core/
Other
215 stars 97 forks source link

ebay-infotip: rename to ebay-infotip-button #1765

Open ianmcburnie opened 1 year ago

ianmcburnie commented 1 year ago

Feature Request

Description

Rename ebay-infotip to ebay-infotip-button to align with our new grouping:

Screen Shot 2022-09-22 at 2 21 35 PM

This also gives us the ability to extract out the overlay piece as just ebay-infotip if we wanted to. This would be consistent with how we do ebay-menu and ebay-menu-button

Would need a corresponding change on the Skin side too.

cc-ebay commented 1 year ago

Consider prioritizing "eliminate/reduce unnecessary changes for the library users" vs "align with our new grouping" or "aesthetic consistency in interfaces matters".

Changes in the interface are a particular delicate matter when a library is extensively used in hundreds of repos. These changes just create extra work for the developers with the chances of introducing new bugs with almost no advantage.

During the last 4 years I noticed quite a few of unnecessary changes for the sake of aesthetic consistency.

Components' names, parameters and behaviors are the API interface for who is using these components. There are good patters for design an API contracts and to evolve it. The first good resource that come up on Google is: https://nordicapis.com/api-change-strategy/

agliga commented 1 year ago

Consider to prioritize "eliminate/reduce unnecessary changes for the library users" vs "align with our new grouping" or "aesthetic consistency in interfaces matters".

It is particular delicate matter when a library is extensively used in hundreds of repos. These changes just create extra work for the developers with the chances of introducing new bugs.

During the last 4 years I noticed quite a few of unnecessary changes for the sake of aesthetic consistency.

Components' names, parameters and behaviors are the API interface for who is using these components. There are good patters for design an API contracts and to evolve it. The first good resource that come up on Google is: https://nordicapis.com/api-change-strategy/

Typically we provide auto migrators. So if this change will be done then we will also add a migrator to use this new component. This makes it so you simply have to do @marko/migrate to simply get the new code. This is what we did with expand button when we removed it. We provided an auto migrator to use the new syntax. Libraries change and evolve. It should be fine and not a big deal.

cc-ebay commented 1 year ago

You obviously do not work on a typical team that is responsible of many repos. And it explains why these kind of changes happens so often with little thought about the consequences for who need to use the library.

Consider just a simple application that is using 5 node modules that are used by multiple apps that depends on ebayui-core.

You will have to make first the changes in the 5 node modules repos, and for each of them: You need to create a new major version, update the docs to tell that the new version only work after that specific version of ebayui-core, updating the specific version of ebayui-core in those modules, make the change in the code, maybe the migration works, maybe it has problems (Ex: older version of marko.js), run test locally and fix problems, create a PR, wait until the PR is approved, if you are lucky to have CI setup for that module, wait for the tests to run, and then finally do a npm publish.

Now you have to update the main app or apps, PRs cycle, CI cycle, release cycles...

And BTW, now you have 2 major versions of those 5 modules that may have to be kept updated. One working for ebayui-core@x < breaking-change and ebayui-core@x >= breaking-change. Let say you have to patch it because of a bug. You need to do it in 2 places, 2 PRs, 2 CIs, 2 releases.

And this is a simple example. If you add in the mix that some people forget to update one of the version, that there may be a secondary dependency on another variable (ex: Legacy Marko version and update marko version), the things get exponentially worse.

I worked on teams where the dependent modules were up to 14 and I had to do that process for 4 applications.

There are a few examples of just ONE breaking changes in the history of a library or a language out there. There is a LOT of pain associate with that and a lot of churn.

Do not change interfaces unless it is REALLY necessary.

agliga commented 1 year ago

You obviously do not work on a typical team that is responsible of many repos. And it explains why these kind of changes happens so often with little thought about the consequences for who need to use the library.

Consider just a simple application that is using 5 node modules that are used by multiple apps that depends on ebayui-core.

You will have to make first the changes in the 5 node modules repos, and for each of them: You need to create a new major version, update the docs to tell that the new version only work after that specific version of ebayui-core, updating the specific version of ebayui-core in those modules, make the change in the code, maybe the migration works, maybe it has problems (Ex: older version of marko.js), run test locally and fix problems, create a PR, wait until the PR is approved, if you are lucky to have CI setup for that module, wait for the tests to run, and then finally do a npm publish.

Now you have to update the main app or apps, PRs cycle, CI cycle, release cycles...

And BTW, now you have 2 major versions of those 5 modules that may have to be kept updated. One working for ebayui-core@x < breaking-change and ebayui-core@x >= breaking-change. Let say you have to patch it because of a bug. You need to do it in 2 places, 2 PRs, 2 CIs, 2 releases.

And this is a simple example. If you add in the mix that some people forget to update one of the version, that there may be a secondary dependency on another variable (ex: Legacy Marko version and update marko version), the things get exponentially worse.

I worked on teams where the dependent modules were up to 14 and I had to do that process for 4 applications.

There are a few examples of just ONE breaking changes in the history of a library or a language out there. There is a LOT of pain associate with that and a lot of churn.

Do not change interfaces unless it is REALLY necessary.

So this is why its important to keep up to date with the latest versions on all projects. If all the applications are using the latest versions then there shouldn’t be an issue. If an upgrade happens and you need to do it then of course all apps will use the latest version and no need to backport and fixes to previous versions. Usually when we rename things too we don’t immediately remove it but give teams time to do the changes. Like i mentioned for this: we will support the old syntax until the next major version. Hopefully by then you will have had plenty of time to upgrade That said i have worked on a team with the same setup you mentioned before and have had experience with that. This is why a) auto migrators are helpful and important and b) keeping up to date on all packages and making sure all teams are using the latest versions will have very little issues upgrading

cc-ebay commented 1 year ago

If you do not own some of the modules the cost and the time necessary for the change become even bigger. Consider the time necessary to do these changes, the cost associate with the context switching of the people involved, the frustration of the engineers, the risk of affecting the final user by introducing a bug. I would not be surprise if the final cost of ebay-infotip to ebay-infotip-button kind of breaking change is not ending up being a few hundred thousands dollars overtime for the whole company.

You can argue that a few hundred thousands dollars for a big company is almost nothing. The question is, is it worth it?

Anyway, the bottomline for me is the philosophy should be "introduce breaking changes when it is necessary to make the change. Avoid to make a change if it is not necessary." - Also make these kind of changes very rarely.

If you do not believe me, there are quite a few articles about the cost of mutating the contract of an interface and strategies about how/when to do it or not.

agliga commented 1 year ago

If you do not own some of the modules the cost and the time necessary for the change become even bigger. Consider the time necessary to do these changes, the cost associate with the context switching of the people involved, the frustration of the engineers, the risk of affecting the final user by introducing a bug. I would not be surprise if the final cost of ebay-infotip to ebay-infotip-button kind of breaking change is not ending up being a few hundred thousands dollars overtime for the whole company.

You can argue that a few hundred thousands dollars for a big company is almost nothing. The question is, is it worth it?

Anyway, the bottomline for me is the philosophy should be "introduce breaking changes when it is necessary to make the change. Avoid to make a change if it is not necessary." - Also make these kind of changes very rarely.

If you do not believe me, there are quite a few articles about the cost of mutating the contract of an interface and strategies about how/when to do it or not.

I think a great solution to your problem would to be make all your 20 modules into a monorepos. This would help A LOT. This way you can publish versions no problem with a single command, you can update code easily using a find and replace or a migrator, you can upgrade versions of all your modules and not have to worry about missing one.

Once you have a monorepo all you need to do is to document and make sure that teams are aware of the new versions you publish and what versions they are compatible with. Or maybe you can have a peer dependency of the version you support and make sure people are reading those warning instead of overlooking them. For example, have your plugins have a peer depdency of the current ebayui. Once a new version comes up, bump that peer depdency and make sure your code works with the new version. That shouldn't take more than 15 mins if you have everything in a monorepo.

Now for us, we find this necessary because it follows this new pattern we introduced last version. Is it necessary for teams? No. Is it necessary for our organization of code? Yes. Do teams have an easy path forward through migrations or simply doing cmd + shift + f? Yes. Well then it seems a no brainer.

Lets try to sit down and discuss how we can get your plugins into a monorepo, I believe that will help greatly and then new breaking changes will no longer be an issue.

Let's try to find solutions, not try to prevent us from doing work because it is inconvenient for people.

ianmcburnie commented 1 year ago

@cc-ebay Renaming components or their attributes is not simply about aesthetics. It's about consistency. As library authors and maintainers, consistency is a big deal for us. We do get designers and developers giving us direct feedback about problems caused by inconsistencies. So while you may perceive it as purely aesthetic, we do not.

Also I'm curious as to why you shared an article on Web APIs (https://nordicapis.com/api-change-strategy/)? We are building a library API, so the majority of that content is not relevant at all. Anyhow, I gave it a read, and here is my feedback on the few parts that were:

If your software has a fuzzy or seemingly random versioning scheme — or no apparent version number at all — you'll likely agree it can be a nightmare to work with.

I agree! But it doesn't.

Versioning schemes like Semantic Versioning are therefore hailed by software professionals for being clearly defined and intuitive. Having a good versioning scheme makes life easier for everyone.

I agree! We have semantic versioning. It's great.

(For libraries that are statically linked at compile time) you develop your client application in accordance to a clearly defined API, link to the version number of the API you developed against, and you have the confidence that the integration is going to work until you upgrade the library.

Yes, I couldn't have said it better myself. I hope your team is following this practice.


Anyhow, at the end of the day, I would prefer to use this ticket to discuss whether we want to make this specific change or not. Is it helpful for the consistency of the system or not?

cc-ebay commented 1 year ago

About the mono repo strategy

Keeping peer-dependencies updated

Or maybe you can have a peer dependency of the version you support and make sure people are reading those warning instead of overlooking them.

People should pay attention to warnings and when there is a discipline of updating peer dependencies, looking at warnings and working to solve them, this could have helped noticing that there is an incompatibility problem. But just checkout any repo to see how many warning are generated when you install the node modules (most of them used to come from platforms modules). The app still works and who created the modules does not spend time to fix the peer-dependency. I cannot make sure "make sure people are reading" across the organization. Especially when key modules are generally not keeping peer-dependencies updated.

A proper use of peer-dependencies, though, does not change the amount of work to update the codebase. It may actually increase it.

The overhead

To make it clear, the codemods to update the code are useful and I love that you take the time to work them in when the changes are more involved. But the overhead is in the development process, and the added complexity it takes to keep multiple versions of a depending modules working for different versions of the depending modules.

A little tangential: Things gets even more complicated and frustrating when CSS overrides are involved. I think the CSS of the library should be consider private data and should not be modified. BUT, there is no mechanism to prevent it and moreover internally it looks like people have a different philosophy choosing the quickest path. Unfortunately components that do not often follow the open-close principle make it necessary to make overrides.

What I am asking

I am not saying the library API should not be open for improvements. I am saying that among the principles that guide the development of the library, there should be a properly prioritized "striving for maintaining backwards compatibility" principle. And moreover the key question is "is the hassle worth the benefits for the end user of the API?"

It is great to use semantic versioning. But it does not mean that "striving for maintaining backwards compatibility" is out when a major version comes out.

Updating a name like this becomes then a tradeoff between "keeping consistency in the component naming" and "making a breaking change".

ianmcburnie commented 1 year ago

We make decisions on a case-by-case basis on how long we will keep supporting a deprecated feature. Just like we will do for this change if we decide to move ahead with it.

I have witnessed first hand several UI libraries at large companies collapse and fail due to mounting inconsistencies, bloated code and technical debt that becomes unmanageable for everybody. I agree we may sometimes appear to be more on the aggressive side of staying on top of that, but so far it has prevented us from those pitfalls.

Updating a name like this becomes then a tradeoff between "keeping consistency in the component naming" and "making a breaking change".

You are preaching to the choir here. We consider these kind of tradeoffs on a continuous basis. It is our job. It is not something we take lightly. We have design systems and accessibility systems that also come into play. Keeping consistency across all systems is important.

But it does not mean that "striving for maintaining backwards compatibility" is out when a major version comes out.

We are striving to maintain backwards compatibility for a reasonable amount of time, even across major versions (we have many examples of this), but we are not striving for maintaining backwards compatibility till the end of time. There are various reasons for that (which I won't get into here). As mentioned above, sometimes we might be more aggressive on moving things out more quickly than others.

cc-ebay commented 1 year ago

It seems that we are aligned. I would keep in mind: "is the hassle of this breaking change worth the benefits for the end user of the API?"

Some other resources about the impact of breaking changes and consequences:

The deeper a library is in the dependency tree, the higher the inconsistency cost, regardless of the magnitude of the direct change cost. Consequently, a library should try to maintain API stability as long as possible.

cc-ebay commented 1 year ago

Finally regarding this issue. Assuming "striving for maintaining backwards compatibility" principle is accepted and prioritized, there is a simple way to "Rename ebay-infotip to ebay-infotip-button" without breaking changes.

Assuming all the rest stays the same, you can simple create rename it and create a ebay-infotip to ebay-infotip-button and mark it as deprecated. BUT this can stay in the API as long as "forever" if you want. Or until nobody else is using it anymore.

It is not always this easy, but the idea here is trying not to have to break the backward compatibility, as long it is counterproductive.

ianmcburnie commented 1 year ago

"Rename ebay-infotip to ebay-infotip-button" without breaking changes. Assuming all the rest stays the same, you can simple create rename it and create a ebay-infotip to ebay-infotip-button and mark it as deprecated.

Yes, we have leveraged this exact strategy for years now. You can look through the history for examples. We are very familiar with it.

Staying in the API "forever" can have consequences beyond the CoreUI codebase. On the Skin CSS side, for example. But again, we have strategies to mitigate that when we deem it necessary.

agliga commented 1 year ago

For now, we will include this in the next minor version. We will rename it to use the new name and create a migrator to still support the old name until next major.