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

Fixes frontend-maven-plugin snippet in basic module. #130

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

Conversation

UgmaDevelopment
Copy link

It looks like, to maintain sanity, the configuration for the
frontend-maven-plugin was defined in the root tutorial pom and the
configuration is inherited by all the submodules, like basic.

The problem was that one following along with the project needs that
configuration.

Now, we show the full configuration instead by including the
frontend-maven-plugin configuration from the root POM instead of
from basic's POM.

Should resolve some concerns described in issues #124, #128.

It looks like, to maintain sanity, the configuration for the
frontend-maven-plugin was defined in the root tutorial pom and the
configuration is inherited by all the submodules, like `basic`.

The problem was that one following along with the project needs that
configuration.

Now, we show the full configuration instead by including the
frontend-maven-plugin configuration from the root POM instead of
from `basic`'s POM.

Should resolve some concerns described in issues  spring-attic#124, spring-attic#128.
@UgmaDevelopment
Copy link
Author

UgmaDevelopment commented Jan 14, 2021

It makes sense why there's been some people confused about this plugin.

What they were seeing:
image

What they needed to be seeing:
image

@UgmaDevelopment
Copy link
Author

Can you think of anything that would stop this PR from being merged?

@mtravnicek
Copy link

@gregturn
Well this actually fixes nothing i dont even see any relevant file being commited...
But what needs to be fixed so in basic and other modules will frontend:install-node-and-npm goal work in Idea
is that in main pom.xml must nodeVersion and npmVersion tags be moved to upper plugin configuration section
just where <installDirectory>target</installDirectory> resides.
I dont get it why you didnt tested it, or does it work from plain mvn commandline?
Anyway great tutorial but this is broken.

@@ -249,7 +249,7 @@ This section contains the barebones information to get the JavaScript bits off t
====
[source,xml,indent=0]
----
include::pom.xml[tag=frontend-maven-plugin]
include::../pom.xml[tag=frontend-maven-plugin]
Copy link
Author

Choose a reason for hiding this comment

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

Hi, @mtravnicek, sorry for the misunderstanding.

i dont even see any relevant file being commited...

It's a subtle change, so I understand why you missed it. Let me see if I can shed some light on the problem.

The tutorial has snippets that refer to actual code. The full configuration for the frontend-maven-plugin used to live in the basic module's POM, but at some point, that configuration was pulled up to the parent POM so that it could be shared with the other modules.

When that configuration got pulled up, it didn't break the functionality of the basic module, but it did break the tutorial, because those following along now can't tell what to put for configuration in the <plugin> element—all they get is a blank plugin configuration.

image

To fix the problem, I told the tutorial to look at the parent POM (../pom.xml) instead of the POM in the basic module (pom.xml). It even already had the tags delineating the plugin section.

Thanks for commenting! If you're seeing a problem I'm not, feel free to clarify further.

Copy link
Author

@UgmaDevelopment UgmaDevelopment Jan 21, 2021

Choose a reason for hiding this comment

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

@mtravnicek, when I run the basic module, from the original repo, it works as expected. Does it work for you?

@gregturn gregturn changed the base branch from master to main April 27, 2021 14:42
@benhunter
Copy link

Is this PR still being considered? It would be extremely helpful to update the Spring Guide: Accessing JPA Data with REST with this change. Thank you!

@zlelik
Copy link

zlelik commented Sep 20, 2023

This is still a problem in 2023 and the tutorial does not work :(

@lions-dan
Copy link

@UgmaDevelopment Thanks Ugma; this should have been Incorporated

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

Successfully merging this pull request may close these issues.

5 participants