-
-
Notifications
You must be signed in to change notification settings - Fork 2k
[OpenWebBeans] Add ObjectFactory for Apache OpenWebBeans #971
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
Conversation
159ec4c
to
74325ea
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
Still relevant |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, not sure how this was forgotten but the DeltaPike PR tickled my memory. Could you add a README.md to explain how this module should be used? It doesn't appear to be trivial.
I general the information included should be sufficient to get some one who's checked out the https://github.com/cucumber/cucumber-java-skeleton to get started.
So listing the required dependencies, the optional dependencies, and including an example of a step definition class that uses injection should be enough. Feel free to add anything else that is note worthy too.
|
||
@Override | ||
public void stop() { | ||
synchronized (contexts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stop
will not be called concurrently. There should be no need to synchronize on contexts.
final CreationalContextImpl<Object> creationalContext = bm.createCreationalContext(null); | ||
T created = type.cast(bm.getReference(bean, type, creationalContext)); | ||
if (!bm.isNormalScope(bean.getScope())) { | ||
synchronized (contexts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. getInstance
will not be called concurrently.
package cucumber.api.openwebbeans; | ||
|
||
public interface OpenWebBeansConfig { | ||
boolean userManaged(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not happy about this. We currently don't have a good way to configure ObjectFactories
. But that doesn't mean we should allow add-hoc configurations. I would prefer to keep this simple and working with sane defaults for now. If people want something more configurable they can implement their own version and deal with that complexity.
<groupId>info.cukes</groupId> | ||
<artifactId>cucumber-jvm</artifactId> | ||
<relativePath>../pom.xml</relativePath> | ||
<version>1.2.5-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs an update. 😆
<parent> | ||
<groupId>info.cukes</groupId> | ||
<artifactId>cucumber-jvm</artifactId> | ||
<relativePath>../pom.xml</relativePath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relativePath
is not needed.
<dependency> | ||
<groupId>info.cukes</groupId> | ||
<artifactId>cucumber-jvm-deps</artifactId> | ||
<scope>provided</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency cucumber-jvm-deps
is not needed.
<dependency> | ||
<groupId>info.cukes</groupId> | ||
<artifactId>gherkin</artifactId> | ||
<scope>provided</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency gherkin
is not needed.
<dependency> | ||
<groupId>javax.enterprise</groupId> | ||
<artifactId>cdi-api</artifactId> | ||
<scope>provided</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored the various POM files to reduce the coupling between modules. You'll have to provide a version for the CDI API here. For reference:
https://github.com/cucumber/cucumber-jvm/blob/master/weld/pom.xml
<dependency> | ||
<groupId>net.sourceforge.cobertura</groupId> | ||
<artifactId>cobertura</artifactId> | ||
<scope>test</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
@@ -0,0 +1,4 @@ | |||
package cucumber.runtime.java.openwebbeans; | |||
|
|||
public class OpenWebBeansObjectFactoryTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty class?
@rmannibucau do we still need this with #1616? |
@mpkorstanje yes and no. Since this PR CDI (v2) has a standard API for standalone apps (SeContainer) so this can be a portable replacement for openwebbeans and weld. CdiCtrl of deltaspike was for before this time - which means that using deltaspike and CDI class explicitly is likely unintended. Im fine closing the or if CDI Se API replaces it but i cant always use deltaspike in the testing stack - i want to control the boot api for cucumber. |
So you are saying that the implementation in #1616 is using CDI incorrectly? Could you put that comment in there. I don't know enough about the CDI ecosystem to make definitive statements.
Figured. |
Commented. |
Sry to ask it her. But what do you want control until boot? Is CDIExtensions (https://docs.oracle.com/javaee/6/api/javax/enterprise/inject/spi/Extension.html) mechanism not enough? |
No for these cases:
So nothing strictly wrong with DS but some advanced usages requiring it to not be there. |
for 1: Producer? for 2: oh i thought i was the only one who need it. How do you solve it? I use scopes for it and think about it for the other pr - but unsure .... |
I'm happy to take both PR's. While CDI containers are used only by a fraction of the users they're relatively low maintenance and have a relatively high number of contributors. I do however have a rather limited knowledge of either so I'd like both implementations to be correct, in line with the frameworks expected use (so as to limit future questions/issues) and focused on the basic use case. For advanced use cases I expect that people re-implement the factory. At least until we come up with something better. Also if you can both review and approve each others PR's then I can have some confidence that both implementations are good. |
|
closing as being superseeded by #1626 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
1 similar comment
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
An alternative to Weld supporting already running container to smoothly integrate with OpenEJB if needed - and with the rule PR I did earlier.
Side note: the PR uses cucumber.api.openwebbeans.OpenWebBeansConfig as a SPI cause cucumber doesn't provide a "config scanning" out of the box but it would have been nice to have it as an annotation.