Implementation of review comments for classpath and jar #196
Conversation
resourceFactories
|
OpenNTF » JavascriptAggregator #453 SUCCESS |
| */ | ||
| public class JarResourceFactory extends ClasspathResourceFactory { | ||
|
|
||
| } |
There was a problem hiding this comment.
I had to add this class file for JarResourceFactory separately although the logic lies entirely in ClasspathResourceFactory. The reason is I cannot put two schemes that can be handled by a factory class. So ClasspathResourceFactory has scheme as classpath and JarResourceFactory has scheme as jar. If we can put comma separated values as scheme for resource factories, we need additional code change in newResource(URI uri) method in AbstractAggregatorImpl to handle it. Please suggest.
There was a problem hiding this comment.
But you can add two schemes that are handled by the same resource factory. See BundleResourceFactory, which handles bundleentry, bundleresource and namedbundleresource schemes.
There was a problem hiding this comment.
This is related to the way we are handling extensions in non-osgi environment. As you know, we are externalising the properties associated with an extension in a properties file - so there are two properties files whose name is com.ibm.jaggr.core.impl.resource.ClasspathResourceFactory.properties and com.ibm.jaggr.core.impl.resource.JarResourceFactory.properties. In these files, we are having scheme as classpath in the first and jar in the second.
So if we are going to use the ClasspathResourceFactory for two schemes, we need to have a method to mention comma separated values for the scheme types.
There was a problem hiding this comment.
I think that would make sense. Doesn't seem like it would be too difficult to implement.
|
OpenNTF » JavascriptAggregator #456 SUCCESS |
| /** | ||
| * An implementation of {@link IResource} for Jar resources | ||
| */ | ||
| public class JarResource extends ClasspathResource { |
There was a problem hiding this comment.
This is good, however, now that we've gotten to this point, I'm failing to see what the value is of having a separate class for jar resources. Why not just add a constructor argument to ClasspathResource for setting the scheme property and use that class for both types of resources?
ClasspathResource
|
OpenNTF » JavascriptAggregator #457 SUCCESS |
| * @see com.ibm.jaggr.service.modules.Resource#getPath() | ||
| */ | ||
| @Override | ||
| public String getPath() { |
There was a problem hiding this comment.
This still doesn't look right. If zipFileEntry specifies a jar file name (e.g. jarfile!/pathComponent), then getPath() needs to return only the part after the !/ separator. The jar file name is not part of the path, just like a host name is not part of an HTTP resource path.
There was a problem hiding this comment.
The variable zipFileEntry is initialized in the ctor of ClasspathResource in such a way that it is actually the path after the jar name. So we are good here.
|
OpenNTF » JavascriptAggregator #463 SUCCESS |
|
Removed JarResourceFactory by adding support to mentioning comma separated schemes as scheme value for resource factory extensions in jaggr-web. |
|
I think this change is required. Make the resourceMap variable static in the class ClassPathResourceFactory. This map contains the mapping of a jar file name with the names of the files and folders when the jat file is expanded. We do not want to calculate this variable repeatedly with every instance of this class. This class is intantiated with the call of newInstance(URI) of class ClasspathResource. |
|
|
resourceFactories