Extract methods to dom package

[oandregal]

Extract isWithinBounds and getPosition methods to dom package from the DropZoneProvider component.

[gziolo]

Nice refactor 👍

[ellatrix]

Are there more use cases for these methods?

[oandregal]

Are there more use cases for these methods?

Not that I can think of at the moment. This is the result of a tidy up phase before I dig deeper into DnD for galleries, so there may be or may not be more consumers to this. Perhaps also plugins could benefit from this, but I don't know yet.

I strongly think it make sense to abstract this logic for the reasons I've mentioned above, though. Both @aduth and @ellatrix have shown concerns over this belonging here. Do you think it should live elsewhere? Although I think the dom package would be a good host to this, I'm also happy to move them to other place.

sorry, I'm on mobile and closed this accidentally before I had finished my comment

[ellatrix]

I'm more concerned about the maintenance cost of adding functions which are only used once to this package. At least if there's some reuse of it, it surer that it's pretty solid and is suitable for more reuse.

[oandregal]

I'm more concerned about the maintenance cost of adding functions which are only used once to this package. At least if there's some reuse of it, it surer that it's pretty solid and is suitable for more reuse.

I agree that would give us more confidence. My thinking, though, is that the domain something operates on is at least equally important an indicator than the number of consumers. Case in point: half of this package methods have only 1 consumer! But: all of them deal with some DOM quirk. Despite they're only consumed once it still makes sense to bundle them together as to: 1) offer a higher-level API than the DOM offers and 2) improve discoverability.

[ellatrix]

Maybe it’s just me, but I still don’t think it’s a good reason. Half of the ones are only used by one package indeed, and I think we made a mistake to put them in the DOM package and expose them. I think in the future the edge functions will go away in the writing flow component, and we’ll have to keep them here, totallinf a fairly significant amount of code. It’s also been recently refactored, and we cannot remove the old functions.

In those case we didn’t really explicitly want to add them, it’s just how the folder and package evolved.

[oandregal]

I thought that when we deprecated something it was removed after two releases. Just checked the handbook and it doesn't state anything, though.

Anyway, I want to be sensitive of everyone's time, so don't want to keep you in this small refactor. Let's have this as a private-only API as a start, we can always revisit later.

[aduth]

I thought that when we deprecated something it was removed after two releases. Just checked the handbook and it doesn't state anything, though.

It originally stated "for two minor releases". Appears it was removed (accidentally?) in #9925

Notably, this applies to the plugin. There is no deprecation timeline for features which are introduced to core.

Some related discussion: https://make.wordpress.org/core/2018/11/14/technical-organisation-post-5-0/

Anyway, I want to be sensitive of everyone's time, so don't want to keep you in this small refactor. Let's have this as a private-only API as a start, we can always revisit later.

I think the motivation behind the proposal is a valuable one in expressing the intentionality of our code in ways which can often be broadly applicable. I think we should encourage this in how we write code, seen in the "before" case with having utility functions separate from the component in which they're used. It's not always a clear line, but I didn't personally find these to be so broadly applicable as to justify being made publicly available and maintained.

[oandregal]

Notably, this applies to the plugin. There is no deprecation timeline for features which are introduced to core.

Oh, I didn't know the difference, thanks for clarifying.

That new knowledge brings new questions, though :) Read through the release doc process and my understanding is that a Gutenberg release looks like:

• we publish the packages to npm
• we take those the assets published to npm and somehow update the WordPress core assets at wp-includes/dist/

So, the question is: how do we make sure that something that was removed in the plugin isn't removed in core?

[aduth]

@nosolosw In practice, there's few things which are actually ever removed. The vast majority of deprecations from v5.5 back to v5.2 were plugin-specific functionality. This isn't very sustainable in the long-term for reasons as outlined in the aforementioned blog post, but it's more-or-less where things stand. Some exceptions may include: (a) build and testing tooling (since they're only available to NPM and can be subject to SemVer major version breakage) and (b) unstable/experimental APIs.

[aduth]

It originally stated "for two minor releases". Appears it was removed (accidentally?) in #9925

I propose adding it back in: #14756