Skip to content
This repository has been archived by the owner on Jan 22, 2024. It is now read-only.

Non-osgi sample of jaggr, jaggr-web #183

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

projsaha
Copy link
Contributor

@projsaha projsaha commented Jun 8, 2014

No description provided.

@buildhive
Copy link

OpenNTF » JavascriptAggregator #418 SUCCESS
This pull request looks good
(what's this?)

@projsaha
Copy link
Contributor Author

projsaha commented Jun 9, 2014

Hi, I was thinking if we can put the code for jaggr-web and web sample in place in the main stream after review. This code wont be part of the build yet and it wont affect the build and osgi sample.

@chuckdumont
Copy link
Contributor

@ddumontatibm - I don't like duplicating files like js/css.js, js/bootstrap.js, etc. Does maven support the concept of file links, so that we can have only one copy of these files, yet they will appear in both the osgi and webapp bundles in eclipse?

@hcldan
Copy link
Member

hcldan commented Jun 10, 2014

They can be included from the main artifact where they live during the build phase. You won't see them in your workspace when you're working in the project, but at build time they can be included in the resulting artifact.

I would avoid duplicating the actual files in source control.

@chuckdumont
Copy link
Contributor

That sounds good. Since Eclipse supports file links, which are implemented as entries in the eclipse project's .project file, all we need to do to continue to support running the samples in eclipse is to add file links to replace the files that we move to main artifact.

}

@Override
public IServiceReference[] getServiceReferences(String clazz, String filter) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring filter will prevent multiple servlets (using different aliases) from running in the same JVM, because the name property of the dictionary is used to distinguish between servlet instances. There are open source ldap filter parsers available, but I think that we should at least provide support for the (name=<name>) filter string which is all that is used by the aggregator. Anything more complicated and you could throw an UnsupportedOperationException.

Never mind. I see now that each aggregator has its own private instance of PlatformServicesImpl, so I guess that the filter can safely be ignored. But this will be a problem if we ever try to support a command console because the console communicates with the aggregator instances that are registered in the service registry, and for that to work, the service registry has to be a global object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So lets not implement the filter implementation in the first cut. For the service registry, lets comment this for now, and we can revisit this after the first cut.

@chuckdumont
Copy link
Contributor

For serviceability, it would be good to add entry/exit logging to the new files. I know that JAGGR doesn't use this consistently, but I've been trying to add it to new files and whenever I make major changes to an existing file. See com.ibm.jaggr.core.util.DependencyList for an example.

configUri = new URI(configUrl.toString());
} catch (URISyntaxException e) {
// TODO Auto-generated catch block
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use logging instead of printStackTrace()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants