Skip to content

Conversation

rwaskiewicz
Copy link
Contributor

@rwaskiewicz rwaskiewicz commented Jun 12, 2023

Issue number: N/A


What is the current behavior?

Ionic Framework references a deprecated object,Context, exposed by Stencil

What is the new behavior?

starting with stencil v4, the deprecated Context object will no longer be exposed by stencil. this change was introduced in stenciljs/core#4437, and will be present in the first v4 prerelease following v4.0.0-beta.2. in anticipation for this change, we seek to remove references to Context early.

Does this introduce a breaking change?

  • Yes
  • No - To the best of my knowledge

Other information

ATM, the Stencil v4 nightly build is passing, as it's grabbing @stencil/[email protected]. The change in which we remove the Context object will occur in the next pre-release. I'm removing Context here to try to get a jump on things.

The current iteration of the code that I'm deleting was added in c415bbe#diff-ce62e75f0c31a76aac491f13a64e9c7771a6cbae8ca6635541164b69f0479bf1

starting with stencil v4, the deprecated `Context` object will no longer
be exposed by stencil. this change was introduced in
stenciljs/core#4437, and will be present in
the first v4 prerelease following v4.0.0-beta.2. in anticipation for
this change, we seek to remove references to `Context` early.
@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label Jun 12, 2023
@rwaskiewicz rwaskiewicz marked this pull request as ready for review June 12, 2023 13:44

const doc = window.document;
const win = window;
Context.config = config;
Copy link
Contributor

Choose a reason for hiding this comment

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

In c415bbe#diff-ce62e75f0c31a76aac491f13a64e9c7771a6cbae8ca6635541164b69f0479bf1, I also see references to DomController. This util also has references to some Ionic.queue variable.

Is the queue related to this context change at all?

Copy link
Contributor

@liamdebeasi liamdebeasi Jun 12, 2023

Choose a reason for hiding this comment

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

Originally added in 6a31f39. However, I don't know how ionic.queue gets set.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Ionic.queue is the result of getContext when context is queue.

I am seeing only a reference to it being set in the compiled code for screenshot compares on Stencil's repo:

(this.queue = o(this, "queue"));

Source: https://github.com/ionic-team/stencil/blob/main/screenshot/compare/build/p-227a1e18.entry.js (hasn't been updated in 3 years).

A feature removed at some point perhaps?

Copy link
Contributor

@liamdebeasi liamdebeasi Jun 12, 2023

Choose a reason for hiding this comment

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

Hmm so it looks like this could change the DomController behavior then. Is there an alternative queue implementation that Stencil implements?

It also looks like getQueue falls back to RAF:

return {
read: (cb: any) => win.requestAnimationFrame(cb),
write: (cb: any) => win.requestAnimationFrame(cb),
};
. Maybe this is an acceptable fallback and we can look to deprecate DomController? If it's just doing an raf then that's not providing much value since developers can already do that today with much less code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think @sean-perkins is right - this was likely functionality that was removed (and is only findable in the Stencil repo due to checked in build artifacts).

I don't know quite enough about Ionic's architecture to really understand how/when this service (DomController) gets used. In the test subdir, I see it gets injected into the ctor of the ProvidersComponent, but that seems to only act as a sentinel to tell if the component is loaded. I also seeing it getting exported here, but I'm not sure why.

That is all to say, I'm not fully sure how DomController gets used here, which makes it tricky to figure out for sure if this is actually affecting the behavior or not.

Are there any docs (internal/external) you could point me to to give me a better sense of it all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is DomController used? I see it in the old diff @liamdebeasi linked, but I don't actually see a reason why it exists. It looks like a dead Angular provider.

IMO I think the recommended changes here make sense, unless I'm missing where DomController is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an old API from the v3 days that got ported over for v4: https://github.com/ionic-team/ionic-v3/blob/cf35d5eb7f38d088e7c2588e42738228c39e9964/src/platform/dom-controller.ts#L57. The goal of the DomController was to provide a way of manipulating the DOM while avoiding layout thrashing. It's a public API that was never documented. However, it has been used in some community posts (https://ionic.io/blog/building-interactive-ionic-apps-with-gestures-and-animations).

Am I correct in my understanding that the underlying queue functionality has already been removed, meaning DomController is currently just using raf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I correct in my understanding that the underlying queue functionality has already been removed, meaning DomController is currently just using raf?

As far as I understand it (based on this conversation), yes. I don't see any assignments to Ionic.queue or any calls to Stencil's getContext that (Sean linked) in the Framework codebase

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool. In that case I'm fine with the proposed change. Sean and I should discuss whether or not to deprecate the DomController since it's really a wrapper around raf at this point.

@rwaskiewicz rwaskiewicz added this pull request to the merge queue Jun 14, 2023
Merged via the queue into main with commit d3232dc Jun 14, 2023
@rwaskiewicz rwaskiewicz deleted the rwaskiewicz/context branch June 14, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants