Introducing node.js to get all features demonstrated in the project.

[AnselmHa]

No description provided.

[HitmanInWis]

Thanks for the PR @AnselmHa!

I'm currently swamped with other matters, but I will review this PR as soon as I have some availability.

[AnselmHa]

Ok, thanks. I'm preparing a second one, maybe you find time for both then.

[HitmanInWis]

Ideally these would be submitted as two separate PRs to be reviewed and merged individually. I can review/test these two new features simultaneously if you prefer since they're currently together on your branch, but going forward it would be best to submit this type of thing as two separate.

[AnselmHa]

I agree. I was not able to two part two without part one. Next one is separated :)

[HitmanInWis]

Reviewing this now. Can you please help me understand the benefit of this new approach vs. the previous approach?

I agree that some folks may be pretty familiar with doing things from node, but the overall execution of the tool is already pretty simple - build the jar with a single maven command (all AEM devs already have maven), copy the jar to your project, and configure the JSON file.

My main concerns with what we'd have with this PR:

• We now have two ways of building the jar - with maven directly or with node (which delegates to maven) so we've definitely increased the complexity of the README and the perceived complexity of using the tool. If the tool looks complex, some people will be scared to try it.
• The two ways of using the tool (direct vs node) also have different paradigms of where the files are generated and how they get into your project, further propagating confusion
  -- With direct use, you have the jar and data-config.json in your code project, and the code gets generated exactly where it needs to be and can be compiled and committed with no additional manual steps
  -- With node use, it appears the code is being generated within the generator project itself, and then needs to be manually moved to the code project. Not only is this different and thus needs different README docs, but I fail to see how this is an improvement.

If node is truly buying us big benefits vs. the current usage, I'd entertain a discussion of whether or not it is uniformly better and should become the new way for everyone. Otherwise, if this is just a personal preference that is different but not necessarily better than the current usage I think the node build option would more appropriately remain in a fork.

Side note, if you want to create a separate PR for the code that prevents file renaming if the generated file has not changed, I can definitely review that one while this issue is being discussed.

[HitmanInWis]

Setting review to "request changes" while we discuss.

[AnselmHa]

Thanks for taking time to discuss this. I guess we talk here about the perspective of the generator and different use cases within the README.md.
Use cases in READEME.md:

1. Experience aem-component-generator protect

• From my perspective as a developer deciding to use the aem-component-generator, it would be good if aem-component-generator is self-contained so no need of copying anything in my project to see what it does.
• Contribute to aem-component-generator would be easier especially for front-end developers if you can run and test the generator with three easy double-clicks on node-Tasks.
• Developers have different skills and are working in several projects, what I'have seen done in node.js by different Developer types (front-end / back-end) together tell me that nodejs is definitely worth to be integrated. By the way, my first groovy-Project was triggered by an maven-plugin adaption, haven't see it before but use it since then.

2. Explaining the data-config.json

• This is very important and should not be in the root readme, but in doc/Explaining-data-config.md
• Having "Explaining-data-config.md" makes it easier to tell how to integrate aem-component-generator in another protect.

3. Integrating aem-component-generator in another protect

• aem-component-generator needs a maven-plugin deployed to maven central then usage is easy.
• As long as no maven plugin is available just copying and run a jar doesn't mean to be easy. I don't see projects acutally doing this in modern times.
• So node.js is maybe just a quick possibility to integrate aem-component-generator in another protect, maven-plugin would be better.

Long story short, I changed the following:

• separate "doc/Explaining-data-config.md"
• separate "doc/Integration-with-node.js" from the README.md "### Step 7: Inspect and copy source files below in your project using node.js"
• add a description how to use node with eclipse

[AnselmHa]

@HitmanInWis did you found time to discuss the nodejs integration. In the meantime I added more templating functions... I don't know if this is the way you like too enhance the templating possibilities therefore I added it to same branch so yo�u can test it easily all together. If you like the change I can separate it.

[HitmanInWis]

Hi @AnselmHa.

I've been absolutely buried lately, so trying to fit in reviews when I can. I will get to these, just can't promise exactly when.

However, the best thing you could do would be to separate your changes (now 3) into separate pull requests, because some of the minor changes are easier to approve/merge. I know that's a bit more work on your end to separate these features back out, but generally when submitting features to an open source project it is best to have one pull request per feature. Do you think that's something you can do?

[AnselmHa]

@HitmanInWis yes, of course. I don't how if you are willing to bring this in the project. And there are some dependencies between the changes, so I will split them when I now what you like. Would that be ok with you?

[HitmanInWis]

Here's the co-mingled features I'm seeing in this PR.

1. Add node.js support (unsure if this provides value, happy to further discuss in a PR of its own)
2. Update generation to only rename a duplicate file if the file contents are different (great idea, let's do it)
3. Richtext support (like the idea, but should follow the pattern of other field types)
4. Templating functions (interesting idea but unsure - trying to understand the value of what it actually saves for the developer - either they code it in the config or code it in the HTML file)
5. Tons of refactors/cleanups to existing code (all for it)

Though code cleanup can be good, it makes some of these incremental changes more difficult to validate and keep separate. It might make sense to have a separate PR or even a couple for #5, with no other changes, as prep before doing the rest of these.

Lastly, for your feature #4, I believe a simpler version of this in PR #26 is going to be merged soon, so you may want to wait for that to avoid crazy merge conflicts.

[AnselmHa]

@HitmanInWis ok:

• you just merge Render HTML from fields set in JSON #26, beside I prefer templating in practice
• I extract the refactorings and new readme.md
• I create new pull requests for the other

[HitmanInWis]

Closing this PR in anticipation of separate PRs for the different features.

[HitmanInWis]

Regarding the templating @AnselmHa I'm not saying we can't do both the templating approach AND the simple approach that was already implemented in #26. If they two work together without issue, based on configurations set in data-config.json, I can see merit in supporting both.