Skip to content
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.

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

[BREAKING] Global app removal #6646

Closed
wants to merge 6 commits into from
Closed

[BREAKING] Global app removal #6646

wants to merge 6 commits into from

Conversation

kpal81xd
Copy link
Contributor

@kpal81xd kpal81xd commented May 31, 2024

This PR removes the inclusion of the global app reference and uses the static property on app base to store all application instance references including the active application.

API changes

  • Removes pc.app

Reasoning

When creating projects with multiple references, it is unclear which reference is held in pc.app. This should be acquired by using the AppBase.getApplication method which allows specific applications to be acquired based on canvas id or if no id is provided then the current active application.

@kpal81xd kpal81xd requested a review from a team May 31, 2024 14:03
@kpal81xd kpal81xd self-assigned this May 31, 2024
@kpal81xd kpal81xd changed the title Global app removal [BREAKING CHANGE] Global app removal May 31, 2024
@kpal81xd kpal81xd changed the title [BREAKING CHANGE] Global app removal [BREAKING] Global app removal May 31, 2024
@Maksims
Copy link
Contributor

Maksims commented May 31, 2024

This will likely to break a lot of people's projects.

@leonidaspir
Copy link
Contributor

Oh most definitely it will... counting the number of client and projects of ours we may need to check/update.

@devcem
Copy link

devcem commented Jun 2, 2024

So we need to update 16 active projects of ours after this.

  • What do we gain after this?
  • Is it really necessary?
  • Is it possible to make this change but still people allow to use pc.app?

@erikdubbelboer
Copy link
Contributor

The change makes sense if you want to support multiple playcanvas instances on the same page. I would suggest to keep this backwards compatible by just always making pc.app point to the last created instance. That way it won't break all these projects that only use one instance.

@leonidaspir
Copy link
Contributor

There are so many references to pc.app in the forums, older tutorials and most likely in many user projects 😇. While it's possible to apply a patch just after the engine has loaded for projects that use pc.app, do consider @erikdubbelboer 's suggestion.

@kpal81xd
Copy link
Contributor Author

kpal81xd commented Jun 2, 2024

This will likely to break a lot of people's projects.

Yes but this will be included in a non forced update along with other breaking changes

@kpal81xd
Copy link
Contributor Author

kpal81xd commented Jun 2, 2024

So we need to update 16 active projects of ours after this.

  • What do we gain after this?
  • Is it really necessary?
  • Is it possible to make this change but still people allow to use pc.app?

Removes ambiguity when dealing with multiple applications. Having a global pc.app does not indicate which of these applications it refers to. Again this change will be included in a non-forced engine editor update which you will have to upgrade to by choice.

@erikdubbelboer
Copy link
Contributor

Is non-forced updates a new thing that the editor is going to support? Cause right now we're always forced into all updates.

@Maksims
Copy link
Contributor

Maksims commented Jun 2, 2024

Currently, it is already possible to create multi-app apps, and devs who do that will have references to apps stored and used as required.
So I would say removing pc.app would not affect them at all, but will definitely affect everyone else, like 98% of single-app apps.

Convenience when debugging from console - is a massive.

So if the only reason of removing pc.app is to reduce confusion for multi-apps, then I would suggest to improve docs for multi-app cases, and not remove pc.app.

Is there any other reason for its removal?

@yaustar
Copy link
Contributor

yaustar commented Jun 2, 2024

I also raise concern on this breaking change. Unless it is needed for another feature, we really shouldn't break this for the reasons above

If this does go ahead, can we at least make it a deprecation?

@MAG-AdrianMeredith
Copy link
Contributor

i see where you're coming from, importing app is very dangerous and error prone. Might I suggest an assertion error/warning + deprecation when accessing pc.app when multiple apps are present. You could always deprecate it then and rename it to make it clear what its for e.g. pc.currentApp and throw an error explaing what you need to do when accessing pc.app for old tutorials etc

@LeXXik
Copy link
Contributor

LeXXik commented Jun 2, 2024

I am probably an odd one, but I've never used pc.app. Was it an old way of getting an application instance? Anyhow, I am curious about "other breaking changes" that @kpal81xd mentioned.

@yaustar
Copy link
Contributor

yaustar commented Jun 2, 2024

@LeXXik

#6407
#6584

And actually thinking about it, I think what @kpal81xd means by non forced update is that the next minor/major release is the build with the WebGL 1 support removed

https://forum.playcanvas.com/t/phase-out-of-the-webgl1-support/35705

So the current version of the engine (1.71) will always be supported in the engine going forward

Despite this, I still think that pc.app should not be removed unless it's blocking a feature or consider a polyfill for the editor only perhaps?

@kpal81xd
Copy link
Contributor Author

kpal81xd commented Jun 3, 2024

Currently, it is already possible to create multi-app apps, and devs who do that will have references to apps stored and used as required. So I would say removing pc.app would not affect them at all, but will definitely affect everyone else, like 98% of single-app apps.

Convenience when debugging from console - is a massive.

So if the only reason of removing pc.app is to reduce confusion for multi-apps, then I would suggest to improve docs for multi-app cases, and not remove pc.app.

Is there any other reason for its removal?

pc.app should not be relied on for access to the application instance it should be on the user to correctly manage their application reference as opposed to relying on a global definition of the application. For debugging yes it is useful that is the only reason I would consider leaving it in.

As for affecting other applications this change will be alongside other breaking changes in a non-forced update.

@Maksims
Copy link
Contributor

Maksims commented Jun 3, 2024

non-forced update

Can you please provide a definition of it? There were no such updates before, so is this different version that users can migrate to, is it a separate version that users use one or the other, will it be forced after some time, what is it?

Many people and projects rely on pc.app, and it is up to docs to provide meaningful info on when it is not recommended.

The reason people use it - is because it is useful and convenient. So there should be a strong reason for its removal, otherwise, it just makes devs lives less convenient and introduces change for no technical reason.

@mvaligursky
Copy link
Contributor

I'm not sure I see what this solves. We used to have a private pc.app that would return a last created app.
Here we have a public AppBase.getApplication(id) which returns the same value when no ID is passed in.
So it only changes the interface, but not solve the actual problem of having a global access to the last created app.

I think the goal here should be to remove any way for the user to obtain global/static reference to the current application, as that in any form causes the problem for multi-app users. Where the app is required by the engine (for example the Entity constructor, but also Asset I think and others, the app instance should be provided by the user, instead of the engine using last created application. But that is a lot larger breaking change unfortunately.

@yaustar
Copy link
Contributor

yaustar commented Jun 3, 2024

If the aim is discourage use of this common private API and not blocking a feature, please consider deprecating it with advice what to use instead in the message. It will be much less pressure on user/customer support and less likely to negatively affect developer sentiment, especially those with long running projects.

@kpal81xd
Copy link
Contributor Author

kpal81xd commented Jun 3, 2024

non-forced update

Can you please provide a definition of it? There were no such updates before, so is this different version that users can migrate to, is it a separate version that users use one or the other, will it be forced after some time, what is it?

Many people and projects rely on pc.app, and it is up to docs to provide meaningful info on when it is not recommended.

The reason people use it - is because it is useful and convenient. So there should be a strong reason for its removal, otherwise, it just makes devs lives less convenient and introduces change for no technical reason.

With regards to the forced update I had assumed the community already knew about the split between engine v1 and v2. We will properly outline what this is and how it will work in a later post.

As for pc.app taking @mvaligursky's point into account technically all global access to the current application should be avoided since for multi applications its reference is ambiguous. For single applications however I see the usefulness of having global access to the current application despite it not being the best practise to use.

@Maksims
Copy link
Contributor

Maksims commented Jun 3, 2024

With regards to the forced update I had assumed the community already knew about the split between engine v1 and v2. We will properly outline what this is and how it will work in a later post.

I read every PR here on git, and am very up-to-date with the engine's development, and this is somewhat surprising to hear. I don't think there was any discussion on such, nor blog post. The only mention was about the LTS-webgl1 version to be supported by the editor.

@kpal81xd
Copy link
Contributor Author

kpal81xd commented Jul 1, 2024

Closed due to unnecessary breakage of projects

@kpal81xd kpal81xd closed this Jul 1, 2024
@kpal81xd kpal81xd deleted the app-global branch July 1, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants