plone / plone.patternslib

An add-on to make patternslib patterns available within Plone 5.
https://pypi.org/project/plone.patternslib/
GNU General Public License v2.0
2 stars 5 forks source link

Remove the need to duplicate JS code #11

Closed pysailor closed 5 years ago

pysailor commented 7 years ago

This PR implements #10. Basically, all JS that should be compiled into the bundle needs to be defined via bower, not added locally. Potentially, the mechanism (see src/plone/patternslib/static/Makefile) needs to be enhanced still to compile CSS as well. I assign this to @thet as the most recent commiter for review.

thet commented 7 years ago

I'll review next week!

Without checking the changeset in detail, I can comment this beforehand:

I think we need to keep all bower managed dependencies. I don't see a duplication of code here.

thet commented 7 years ago

There was some discussion @ plone conf. IMO, we should keet as it is. The registry.xml files including all resources are very valuable in the usecase, when you want just depend on a subset of all patternslib patterns. That wouldn't be supported in the future otherwise. But maybe I missed some other discussion. What do you think @pysailor ?

pysailor commented 7 years ago

OK, I'll try to summarize our discussion we had at ploneconf:

@thet, I think these are the additional use cases you have:

Therefore the suggestion was to keep the current (unfortunate) duplication of code, i.e. the complete JS code of Patternslib/Patterns plus additional custom JS code that lives outside of Patternslib like pat-leaflet. That way, all the individual packages can still stay registered in the registry.cml

In this case, we need a good mechanism for quickly updating this duplicated code in case there are changes upstream (e.g. bugfix in some pattern). Here I think we agreed to use use bower+npm plus a Make file, so that there can be a command like make update or similar that will update the sources.

In any case, the compiled bundle will always be checked in and registered by default, since this the perceived use-case for the standard user.

@thet Is that about correct?

thet commented 7 years ago

yes, that's correct.