Skip to content

chore(global): remove unused context references #27634

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 14, 2023
Merged

chore(global): remove unused context references #27634

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions core/src/global/ionic-global.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import { config, configFromSession, configFromURL, saveConfig } from './config';

// TODO(FW-2832): types

declare const Context: any;

let defaultMode: Mode;

export const getIonMode = (ref?: any): Mode => {
Expand All @@ -22,7 +20,6 @@ export const initialize = (userConfig: IonicConfig = {}) => {

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 •
edited
Loading

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 •
edited
Loading

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:

ionic-framework/angular/src/providers/dom-controller.ts

Lines 33 to 36 in 1f0c804

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.

const Ionic = ((win as any).Ionic = (win as any).Ionic || {});

const platformHelpers: any = {};
Expand Down