Introduced a plugin-API and show usage with rhino-xml

[rPraml]

This is my first try to introduce a plugin-api, so that logical separate code parts (rhino-xml, liveconnect) can register them into the rhino engine.

Before I continue here and invest a lot of time, I would like feedback on whether you think this is going in the right direction

[rPraml]

Tried to remove all XML-Lib dependency to rhino-xml. There is still org.mozilla.javascript.xml.XMLLib and org.mozilla.javascript.xml.XMLObject in rhino base module. They are a bit difficult to remove

[rPraml]

I have removed both checks. If rhino-xml is on classpath, we should also have org.w3c.dom.Node.
Can re-add this check, if wanted.

Note: Kit.classOrNull can be very slow, if class is not found, as every check a ClassNotFoundException happens

[rPraml]

CHECKME: Do we need the difference between safe and unsafe objects in the long term?

[gbrail]

I think that this is the right direction, and we should pursue it.

I know that with these changes, the tests work, because all the tests in the "tests" module will include all the modules.

Can we also add a test in the "rhino" module that will ensure that whatever is supposed to happen happens if you try to use the E4X syntax when the rhino-xml module is not in the classpath?

[rPraml]

I have tested to remove the rhino-xml dependency from tests and examples

A few tests like DoctestsTest or the e4x tests from MozillaTestsuite fail with ReferenceError: "XML" is not defined - all in all we have
145782 tests completed, 538 failed, 12 skipped without rhino-xml on classpath.

I've added a small A/B test. In the long term we might think about to split the tests module in tests and tests-xml

Roland

[gbrail]

Sorry to be dribbling questions out here one at a time but I want to understand the implications of this.

I THINK that what happens here now is that to use E4X features, you need "rhino-xml" in your classpath, AND you need to enable the FEATURE_E4X flag in the context. (I admit, though, that I don't use this feature myself so I don't know.)

I like using the plugin model here, but does the user experience change?

If it won't, then I don't think we should deprecate the FEATURE_E4X flag -- I don't think that the special E4X syntax should be enabled unless you set this feature flag, since E4X requires the special syntax and all that.

[p-bakker]

Not sure if we ought to deprecate/remove the E4X feature flag: even if the 'rhino-xml' is on the classpath, it doesn't mean every Context instance will have/need/is allowed to use E4X

[p-bakker]

Which begs the more general question: just the fact that a plugin is available on the classpath, does that mean it needs to be automatically initialized into each TopLevel?

Or does it do that only in for example the shell and/or by default when using the standard ContextFactory, with the ability for integrations to take control over it, for more fine-grained initialize behavior?

[rPraml]

Sorry to be dribbling questions out here one at a time but I want to understand the implications of this.

All good, it's important to know the impact here.

I THINK that what happens here now is that to use E4X features, you need "rhino-xml" in your classpath, AND you need to enable the FEATURE_E4X flag in the context. (I admit, though, that I don't use this feature myself so I don't know.)

The current implementation (=without this PR) behaves this way:

• it checks here if FEATURE_E4X is set and if there is a E4XImplementationFactory present
• The e4xImplementationFactory is assumed to be present, if "dom3" (org.w3c.dom.Node) is on classpath. This is done here
• It uses the rhino-xml factory by default (regardless of whether it is in the classpath or not)

So, setting the FEATURE_E4X flag without dom3 present, will therefore result in a "XML not defined" error, when code tries to access the XML object. (but extended xml compiler syntax is supported ¹ - so you can compile e4x code, but cannot run it)
When dom3 is present, but no rhino-xml, you'll get a XMLLibImpl-ClassNotFound error

The new implementation (=with this PR) behaves this way:

• When the XmlPlugin (=rhino-xml) is not registered (because it is not on classpath, or you are not using a factory, that loads all plugins with the serviceLoader), there should be no change, if FEATURE_E4X is set or not.
• When the XmlPlugin is registered, then it will do exactly this code, which means E4X is present, if the plugin is present and the flag is set.
• We do no longer check, if dom3 is present or not (this could cause issues in edge cases, see ¹, because you'll get the ClassNotFoundError instead of "XML not defined" error)
• (Not yet implemented: We could change "rhino-xml" to have dom3 as as transient dependency, but this means, "rhino-all" has dom3 also as transient dependenency)

I like using the plugin model here, but does the user experience change?

This change should only affect the edge case, when you set the FEATURE_E4X flag, but no dom3 is on classpath (current implementation can compile e4x code, the new one wont, if rhino-xml is also missing or probably run into an error elsewhere)
So by now, this PR should not introduce a breaking change in most cases.

If it won't, then I don't think we should deprecate the FEATURE_E4X flag -- I don't think that the special E4X syntax should be enabled unless you set this feature flag, since E4X requires the special syntax and all that.

I would enable the e4x syntax and object only, if the plugin is in place. Now you need both, the plugin AND the flag. See here

Do you think, there are use cases, where you want to compile e4x code, but not run it?

When we decide to deprecate the E4X flag, this means, that the user must use a custom context factory and control e4x availability with the plugin presence or not. At this point I am also a little unsure whether we can burden this breaking change to the user

Which begs the more general question: just the fact that a plugin is available on the classpath, does that mean it needs to be automatically initialized into each TopLevel?

By default, the contextfactory loads all plugins from the serviceLoader here

And you can set your own list of plugins with setPlugins

Admittedly, this customization is a bit limited or cumbersome because unmodifiable lists are used to ensure that checkNotSealed etc. will work properly (I do not want to do something like factory.getPlugins().remove(0), you might fetch the current plugins, filter them and write them back)

I'm also unsure if we should get the plugins for each new factory from the ServiceLoader or only for the one created with the default constructor (or the GlobalFactory).

I could imagine that if I call new ContextFactory(emptyList()) I would have a factory without plugins

Or does it do that only in for example the shell and/or by default when using the standard ContextFactory, with the ability for integrations to take control over it, for more fine-grained initialize behavior?

Yes, that's what I would like to do, but the current implementation is probably not 100% suitable for that (see above)
I probably need to make some improvements at this point.

[gbrail]

OK -- looking for more comments here from some of the others, but in general this sounds like a good approach.

However, I started this conversation because I see that the "FEATURE_E4X" flag is marked deprecated in this PR. I think that if we want the above -- namely, to have E4X, you need BOTH to set the feature flag AND have rhino-xml in the classpath -- then we should not deprecate it.

[rPraml]

However, I started this conversation because I see that the "FEATURE_E4X" flag is marked deprecated in this PR. I think that if we want the above -- namely, to have E4X, you need BOTH to set the feature flag AND have rhino-xml in the classpath -- then we should not deprecate it.

Yes, at the moment you still need both because I don't want to introduce incompatibility (= existing code does not compile).

I would prefer if we can remove the flag completely and control the e4x support solely by whether the plugin is registered in the ContextFactory or not. But this means, the current API breaks.

That's the reason, I've deprecated it. A plan could be:

• deprecate the flag. It will work as expected as long as you use the (default) service loader for plugins
• set the flag to "true" as defaut. So e4x will be available, as long as the plugin is registered. The user can disable e4x by disabling either the flag or the plugin (or - of course - both)
• remove the flag. So e4x can only disabled, when plugin is not registered.

I'll be honest here that I can't estimate exactly how big the outcry will be if the API breaks in a 1.7.x version. Maybe we should really wait until 1.8.x or 2.x to remove it

[rPraml]

I've rebased this PR and submitted a different approach how to deal with plugins and autoloader:
99bfb30

By default, the standard-constructor of ContextFactory will create a factory with all plugins, found by serviceloader.

Plugins can disabled with a system propery, so if you set rhino.plugin.xml.disabled (or maybe later rhino.plugin.liveconnect.disabled) to true, the plugin is no longer automatically discovered by the standard-constructor.

Nevertheless, you can still construct your own ContextFactory, where you manually add the XML-plugin.
cf = new ContextFactory(List.of(new XmlPlugin()))

In this use case, you have the standard factory, without e4x support and an explicit one with e4x support (of course, you still have to set the FEATURE_E4X, which I suggest to remove.)

Also the other way is possible:
If no rhino.plugin.XXX.disabled is set, a new ContextFactory() will have the XML-Plugin. And a new ContextFactory(getDefaultPluginsFromServiceLoader().stream().filter(p->!"xml".equals(p.getName())).toList() will construct a contextFactory without the plugin. (Maybe we can add some glue logic to include/exclude certain plugins)

[gbrail]

I think that this is a fine change and we should do it, but I still don't think that we should deprecate this flag until it really does something. IMO, deprecating this flag is like saying that we're going to deprecate the E4X feature and I don't think that we're doing that.

[rPraml]

I think there is still a misunderstanding here...

until it really does something

the flag currently does something, it controls, if E4X is enabled, when the plugin is present. So currently, you have to enable the flag (which is done by default for >= 1.6) and ensure, that the plugin is in classpath. I see these options:

add xml-plugin to rhino-all and keep the flag

If someone uses rhino-all as dependency, he will get the plugin automatically and the flag behaves like before. This solution provides maximum backward compatibility (and this is how it is implemented now, but it is annotated as deprecated)

remove the flag completely

As the flag is enabled by default (at least for languageVersion >= 1.6), we could move that check to the plugin. So if someone has decided to overrride FEATURE_E4X in his factory, this will be a breaking change, as code will no longer compile.

something between the 2 solutions

deprecate the flag - with a clear hint, that disabling E4X (that is the purpose that the flag currently has IMHO) with the flag will be removed in the future and should be controlled with the plugin mechanism.