LPS-86129 Removes temporary SF supression #68246
Conversation
…chine-generated one
|
CI is automatically triggering "ci:test:sf" and "ci:test:relevant" for this pull to run Source Formatter and relevant tests. Comment "ci:test" to run the full PR Tester for this pull. |
✔️ ci:test:sf - 1 out of 1 jobs passed in 2 minutes 42 seconds 734 msClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: pr-1658 1 Successful Jobs:For more details click here. |
liferay-continuous-integration
added
ci:test:relevant - failure
and removed
ci:test:relevant - pending
labels
|
Merged. Thank you. |
|
Merged with minor changes in upstream @jbalsas @izaera @Preston-Crary can you take a look at modules/apps/frontend-js/frontend-js-loader-modules-extender-api/src/main/java/com/liferay/frontend/js/loader/modules/extender/npm/NPMResolverUtil.java you're either going to think it's genius.. or it will make you cringe. But I pushed them to do something like it because they were writing the same class over and over again. Thx |
🤔 I'm not sure what looks strange in that class. The only problem with NPMResolvers is that they are not tight to OSGi lifecyle but to web context lifecycle: they should not appear until a web context and its related NPM objects have been instantiated. So, you have to wait for all of them to be available, and that's what the
Another possibility is attaching the In the end, what happens is that you have two scenarios where you need to use the
Case 1 is typical in scenarios where infrastructure code wants to pull npm modules from the top level user bundle to use them in some place it is rendering. Case 2 is typical in scenarios where an infrastructure bundle contains an npm module that it wants to use to provide some feature to top level user code. However, if you think we can refine it I'm open to feedback. I can think of more ways to fix it but I'm not sure of the final goals (I know we want to simplify it, but I'm unsure about how) so it's difficult for me to decide between the alternatives... :-) |
|
@izaera NPMResolvedPackageNameRegistrar seems backwards to me. It's something the bundle needs but isn't available until the bundle is active, it should be the other way around. The problem is we don't have a good way to handle OSGi services from web and taglib modules since they operate in a different worlds. @shuyangzhou and I have an idea of something we can do to make referencing dependencies from web modules easier. It'll take some time to be sure it will work and more time to implement but in the end it could fix the resolution problems and require less code in web modules. |
|
@Preston-Crary It's not really the Let me explain precisely what the What we need is to resolve an "npm module name" (which is something like Why is that call special? Because The first approach we did was to specify the version numbers directly. So, when we had (in a JSP for example) something like: we wrote: The problem with this was that, whenever you changed the version number of Then, we decided to introduce the After that, because
Only when both are ready, we can assign an This worked for 99% of the scenarios, but then it came the next problem. Even though 99% of the time you want to refer to a JS module inside the same bundle where your code (JSP or Java) lives, sometimes infrastructure code wants to load JS modules not from the infrastructure bundles, but from the bundles calling the infrastructure. Other times, infrastructure code wants to load JS modules from its own modules. And it is very probable that, in a single class of JSP, the infrastructure code wants to refer to both type of modules (the ones it owns, and the ones owned by the calling bundle). This could have been done simply by injecting the Of course, if you call Hope this "short & light" explanation makes it clearer to understand. P.S: there's another possibility we haven't explored, which is making To make this last part clearer, what you have is: Where usually |
|
@izaera thanks for the explanation.
This is the main part we don't like, and it's not unique to NPM. Any statically accessed service used in a web module may have this problem. Another part we don't like is "pulling" based registration because each time you need something you have to ask for it which takes some CPU time. It's much better if we can find a "pushing" based solution where the client code is given what it needs to execute it's purpose. This is a good enough solution for short term and I do not want you to resolve this specifically for NPMRegistry, we want to find a general solution for this problem that can be applied to many other services in web modules. I don't want to go into the details of our plan because we're not sure it'll even work yet. |
|
@Preston-Crary I see :-). I've also been thinking about this issue a lot of times. Perhaps having something similar to OSGi DS but attached to the WebContexts would make it. For example, one could declare what OSGi services need to be attached to the WebContext in the BND files, or in Java classes and then the deployer grabs them and injects them as attributes. It could also make sure that the WebContext does not start unless those services are present. It's not a complete solution, but most of the time people just need singleton services for their JSPs and right now the portal is full of internal My two cents... |
|
@izaera that's very similar to our idea, the main difference is we are looking at preventing web bundles from starting unless they have all their requirements. |
No description provided.