liferay / liferay-frontend-projects

A monorepo containing assorted Frontend Infrastructure Team projects
Other
69 stars 68 forks source link

@liferay-npm-bundler The validation of custom headers is too restrictive #541

Closed mir333 closed 3 years ago

mir333 commented 3 years ago

Hi

would it be possible to decrease the level of validation that is done for the customer header from throwing an Error to just logging a warning/error that? I'm referring to this code.

https://github.com/liferay/liferay-frontend-projects/blob/dc7d947da927de33e556c7570c007325efa560dc/maintenance/projects/js-toolkit/packages/liferay-npm-bundler/src/jar/manifest.ts#L71

The reason is that the Error is too restrictive. I'm having my own extender that will process these bundles but would like to use the js toolkit to create the jar file in the JS code. Whit introducing this check it is no longer possible to change the extender.

As an alternative, it would be nice to have an advanced feature that would allow passing the bnd.bnd file form outside.

izaera commented 3 years ago

Probably not... We are now forbidding customizations to these headers:

    'Bundle-ManifestVersion',
    'Bundle-Name',
    'Bundle-SymbolicName',
    'Bundle-Version',
    'Manifest-Version',
    'Provide-Capability',
    'Require-Capability',
    'Tool',
    'Web-ContextPath',

Some of them could -maybe- be tweaked a bit, but most of them have to have standard values because otherwise the resulting artifact wouldn't be deployable and/or would fail in very unexpected ways because of the assumptions the Portal and the tools make.

For the same reasons, allowing an external MANIFEST.MF is not possible because it would be a very error-prone feature that would make support much more difficult for us.

Finally we cannot use bnd.bnd files, because the JS Toolkit is based on JavaScript and bnd is all about Java.

What is the exact use case here? What header do you want to change, how, and what do you want to achieve?

mir333 commented 3 years ago

Yea I get the above and what you try to prevent and I don't expect that dev will mess around with this if they don't know what they doing as it would not work but would be good to have the override option.

It's a philosophical question of what is better. Putting a console.error that saying "are you sure that you know what you are doing!" is better than just error out and not give any options than forking.

On what I'm doing. I have my own extender that creates the portlet and for that, I need to change the Required-Capability. I used the custom headers to get this in.

 "Require-Capability": "osgi.extender;filter:=\"(osgi.extender=liferay.reactunion.portlet)\""

Recently we were doing maintenance and updated our dependencies and this stopped working.

Yea and you are right about the bnd.bnd file. My mistake, I meant MANIFEST.MF.

izaera commented 3 years ago

It's a philosophical question of what is better. Putting a console.error that saying "are you sure that you know what you are doing!" is better than just error out and not give any options than forking.

Yes, if you only consider the dichotomy, but there may be more intermediate solutions ;-)

For example, in your case it would be enough to make the bundler allow more capabilities in addition to the ones it's configuring by default, then check that the user is not overriding any one of those, but only adding new ones. That, in fact, makes sense as a feature.

In any case, I understand the two different philosophies of "making an extensible tool and rely on the developer" vs "making it opinionated" but I think we will be switching more to the opinionated side in the future, because there are lot of considerations to take into account when making these decisions (not only technical ones but also the cost of support, the main target of the tool, the cost of documentation, ...). However I'm not the owner of the backlog, so that's only my impression.

izaera commented 3 years ago

Turning this a feature request and mentioning @bryceosterhaus and @dsanz so they know about it.

dsanz commented 3 years ago

From the list of forbidden headers, I think the Require-Capability header is one of the few ones where it may be useful to allow some degree of customization, in any case, it should be additive (i.e. respect the defaults already provided by the tooling). The case of osgi.extender is one of them. However, current implementation prevents from adding the same namespace twice to the header value.

@mir333 would your use case require the removal of the ootb extender liferay.frontend.js.portletin favor of the user-provided one, or both extenders would be needed at the same time?

mir333 commented 3 years ago

The portlet extender is instantiating a new Portlet based on the configuration so if we would add another of them I would probably end up with 2 portlets registered one by the default extender and one by the custom one. The customer code could have some things to overcome the name clashes at registration but it would be slightly confusing.

Preferably, there should be an option to pass the whole filter part of the Require-Capability header.

izaera commented 3 years ago

Ah, I see now. I didn't got that you are replacing our extender with yours. I thought you were adding an extra one.

In this case, if you are trying to replace our extender with yours I would suggest replacing the JAR step totally by a custom one of yours. This is because both the JAR creation and the extender in the server are tightly coupled and, if we open one of them to working with a different counterpart, there's a big risk of breaking the setups every once in a while.

Why? Because we will always be evolving/maintaining the JAR feature aiming at our extender so, every change we introduce may potentially break your setup. And, from the opposite side, everyone using custom extenders may potentially file an issue and request support for something that is not part of the product we want to offer.

So, my advice would be disabling JAR creation setting create-jar to false and do your own JAR creator. You just need to get the contents in the build directory (after liferay-npm-bundler is run) and put them in the JAR, it's a straightforward process.

In fact, you can clone our jar modules if you want, or use bnd + jar if you are involving Java tools in your build. That should give you a more robust solution than if we open our JAR creation to a divergent scenario.

We currently disable JAR creation in some of our scenarios, so that's something we support and will continue to support. In fact, the original idea was that the bundler never created JAR files (because it's a bundler, not an OSGi build tool) but we added that when we created the extender (and only aimed at that).

For the rest of cases, namely those where people creates mixed Java/JavaScript portlets, it's the Java tool (in our case blade/Liferay Workspace or liferay-portal's build tools) which create the JAR from the output directory of the liferay-npm-bundler.

mir333 commented 3 years ago

Thanks for the suggestion. I'll try it that way.

izaera commented 3 years ago

Feel free to ask for help if needed :-)