Added support to customizable the `projectFileExtensions`. #19550

oliveryasuna · 2024-06-11T01:11:54Z

Description

Please see the issue.

Fixes #19527

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended. NOT APPLICABLE?
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

vaadin-bot · 2024-06-18T03:56:42Z

TC Format Checker Report - 03:56 - 18 - Jun

There are 2 files with format errors

To see a complete report of formatting issues, download the differences file
To fix the build, please run mvn formatter:format in your branch and commit the changes.
Optionally you might add the following line in your .git/hooks/pre-commit file:
```
mvn formatter:format
```

Here is the list of files with format issues in your PR:

flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FlowModeAbstractMojo.java
flow-server/src/main/java/com/vaadin/flow/server/frontend/Options.java

caalador · 2024-06-18T04:01:28Z

The dev-bundle-plugin also needs to implement the new projectFileExtensions() method

Error:  /home/runner/work/flow/flow/flow-plugins/flow-dev-bundle-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildDevBundleMojo.java:[80,8] com.vaadin.flow.plugin.maven.BuildDevBundleMojo is not abstract and does not override abstract method projectFileExtensions() in com.vaadin.flow.plugin.base.PluginAdapterBase

Also it feels like the extensions should be an addition to the existing and not replacing the list of extensions.

mcollovati

A couple of comments:

I agree with @caalador that the configurable extensions should be added to the default list and should not replace them.
it would be better to define the default extensions in a single place instead of having duplication in Maven and Gradle plugin. FrontendUtils class could be a good host.
I know that in the Vite configuration file, the constant is named projectFileExtensions, but IMO it doesn't sound like a good name for a plugin setting. What about frontendFileExtensions? Could it be a better name?

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateVite.java

oliveryasuna · 2024-06-19T14:53:33Z

A couple of comments:

* I agree with @caalador that the configurable extensions should be added to the default list and should not replace them.

* it would be better to define the default extensions in a single place instead of having duplication in Maven and Gradle plugin. `FrontendUtils` class could be a good host.

* I know that in the Vite configuration file, the constant is named `projectFileExtensions`, but IMO it doesn't sound like a good name for a plugin setting. What about `frontendFileExtensions`? Could it be a better name?

I agree with everything and will make changes later. Thanks for considering this!

… add to existing ones.

mcollovati

The implementation seems broken because of a missing comma in placeholder replacement.
I also suggest adding a couple of tests in TaskUpdateViteTest like the following one

    @Test
    public void generatedTemplate_extraFrontendExtension_addedToViteConfiguration()
            throws IOException {
        options.withExtraProjectFileExtensions(List.of(".svg", ".ico"));
        TaskUpdateVite task = new TaskUpdateVite(options, null);
        task.execute();

        File configFile = new File(temporaryFolder.getRoot(),
                FrontendUtils.VITE_GENERATED_CONFIG);

        String template = IOUtils.toString(configFile.toURI(),
                StandardCharsets.UTF_8);

        Assert.assertTrue(
                "Extra frontend extensions should be added to vite configuration, but was not.",
                template.contains(
                        "const projectFileExtensions = ['.js', '.js.map', '.ts', '.ts.map', '.tsx', '.tsx.map', '.css', '.css.map', '.svg', '.ico'];"));
    }

mcollovati · 2024-07-01T09:09:04Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateVite.java

+                                return "'" + StringEscapeUtils.escapeJava(ext)
+                                        + "'";


It looks like to me that StringEscapeUtils.escapeJava does not escape single quotes, that is the only replacement that we need (e.g. ext.replace("'", "\\'")).

mcollovati · 2024-07-01T09:18:18Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateVite.java

+                            } catch (Exception e) {
+                                throw new RuntimeException(e);
+                            }
+                        }).collect(Collectors.joining(", ")));


The result of this string seems not correct; it will be appended to the existing list without a coma.

const projectFileExtensions = ['.js', '.js.map', '.ts', '.ts.map', '.tsx', '.tsx.map', '.css', '.css.map''.svg', '.ico'];

An option could be the following, for example

String extraExtension = ""; if (options.getExtraProjectFileExtensions() != null && !options.getExtraProjectFileExtensions().isEmpty()) { extraExtension = Optional.ofNullable(options.getExtraProjectFileExtensions()) .orElse(Collections.emptyList()).stream() .map(ext -> ext.replace("'", "\\'")) .collect(Collectors.joining("', '", ", '", "'")); } template = template.replace("#extraProjectFileExtensions#", extraExtension);

mcollovati · 2024-07-01T09:21:06Z

flow-plugins/flow-gradle-plugin/src/main/kotlin/com/vaadin/gradle/VaadinFlowPluginExtension.kt

+     * Hashes are calculated for these files as part of detecting if a new prod
+     * bundle should be generated.
+     */
+    public abstract val extraProjectFileExtensions: ListProperty<String>


I'm still not convinced by the setting name. It still seems too generic to me.
I mean, the purpose is to add extension for frontend files that should be considered, not overall project files.
I propose something like frontendExtraFileExtensions or frontendAdditionalFileExtensions

@mshabarov what do you think?

frontendExtraFileExtensions sounds like a good name, or frontendBuildExtraFileExtensions may be even more descriptive for developer.

mcollovati

Additional note: it would probably make sense that configuration setting does not require the extension to start with a dot. I would prefer a configuration like

<extraProjectFileExtensions>
   <extraProjectFileExtension>svg</extraProjectFileExtension>
   <extraProjectFileExtension>ico</extraProjectFileExtension>
</extraProjectFileExtensions>

rather than

<extraProjectFileExtensions>
   <extraProjectFileExtension>.svg</extraProjectFileExtension>
   <extraProjectFileExtension>.ico</extraProjectFileExtension>
</extraProjectFileExtensions>

It would be better if the dot would be automatically added at runtime, if not already defined in configuration.

mcollovati · 2024-07-01T09:35:59Z

flow-plugins/flow-plugin-base/src/main/java/com/vaadin/flow/plugin/base/PluginAdapterBase.java

+     * Get the list of project file extensions.
+     *
+     * @return list of project file extensions


We should add a note on the expected extension format here (e.g. no initial dot) with a simple example list.

mcollovati · 2024-07-01T09:37:52Z

...ugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FlowModeAbstractMojo.java

@@ -232,6 +233,9 @@ public abstract class FlowModeAbstractMojo extends AbstractMojo
    @Parameter(property = InitParameters.REACT_ENABLE, defaultValue = "${null}")
    private Boolean reactEnable;

+    @Parameter(defaultValue = "${null}")


I would add a property attribute named for example frontend.extraFileExtensions so that it could be specified also on the command line

Also, the field needs Javadoc. IIRC they are used for maven goal documentation

mcollovati · 2024-07-01T09:39:35Z

flow-plugins/flow-gradle-plugin/src/main/kotlin/com/vaadin/gradle/VaadinFlowPluginExtension.kt

+     * The list of extra file extensions that are considered project files.
+     * Hashes are calculated for these files as part of detecting if a new prod
+     * bundle should be generated.
+     */


We should add a note on expected extension format and a simple example list.

mcollovati · 2024-07-01T09:41:35Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/Options.java

+
+    /**
+     * Sets the extra project file extensions.
+     *


We need to provide additional detail on the expected extension format and perhaps a simple example

mcollovati · 2024-07-01T09:51:14Z

@oliveryasuna let me know if you need help in setting up the tests or if you have any doubts

mcollovati · 2024-09-09T11:45:04Z

@oliveryasuna are you still interested in/do you have time to complete this PR?
Otherwise, the Flow team can take care of and mention you as a co-author.

oliveryasuna · 2024-09-09T12:12:40Z

@oliveryasuna are you still interested in/do you have time to complete this PR? Otherwise, the Flow team can take care of and mention you as a co-author.

Hey @mcollovati, sorry, it is completely fell off my radar. If the flow team could finish it off, that would be great! This will allow us and many to simplify our build process.

Added support to customizable the projectFileExtensions.

b74c97b

oliveryasuna marked this pull request as draft June 11, 2024 01:12

oliveryasuna mentioned this pull request Jun 11, 2024

Include .scss and .sass in stats.json hashes #19527

Open

mcollovati added the Contribution PRs coming from the community or external to the team label Jun 11, 2024

mshabarov requested review from mshabarov and caalador June 17, 2024 11:50

Merge branch 'main' into 19527-customizable-project-file-extensions

23df89c

mshabarov requested review from mcollovati and removed request for mshabarov June 17, 2024 11:52

vaadin-bot added the +0.0.1 label Jun 18, 2024

mcollovati requested changes Jun 19, 2024

View reviewed changes

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateVite.java Outdated Show resolved Hide resolved

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateVite.java Outdated Show resolved Hide resolved

Made projectFileExtensions into extraProjectFileExtensions, which…

3b80e08

… add to existing ones.

oliveryasuna requested a review from mcollovati June 24, 2024 13:11

oliveryasuna marked this pull request as ready for review June 24, 2024 13:11

Ran Maven formatter.

6123290

knoobie mentioned this pull request Jun 30, 2024

running vaadin:prepare-frontend using directory dependency works, but jar dependencies filter away resources #19645

Open

mcollovati requested changes Jul 1, 2024

View reviewed changes

mcollovati reviewed Jul 1, 2024

View reviewed changes

mcollovati requested changes Jul 1, 2024

View reviewed changes

mshabarov self-requested a review August 5, 2024 11:48

mshabarov removed their request for review September 9, 2024 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support to customizable the `projectFileExtensions`. #19550

Added support to customizable the `projectFileExtensions`. #19550

oliveryasuna commented Jun 11, 2024

vaadin-bot commented Jun 18, 2024

caalador commented Jun 18, 2024

mcollovati left a comment

oliveryasuna commented Jun 19, 2024

mcollovati left a comment

mcollovati Jul 1, 2024

mcollovati Jul 1, 2024

mcollovati Jul 1, 2024 •

edited

Loading

mshabarov Sep 9, 2024

mcollovati left a comment

mcollovati Jul 1, 2024

mcollovati Jul 1, 2024

mcollovati Jul 1, 2024

mcollovati Jul 1, 2024

mcollovati Jul 1, 2024

mcollovati commented Jul 1, 2024

mcollovati commented Sep 9, 2024

oliveryasuna commented Sep 9, 2024

Added support to customizable the projectFileExtensions. #19550

Are you sure you want to change the base?

Added support to customizable the projectFileExtensions. #19550

Conversation

oliveryasuna commented Jun 11, 2024

Description

Type of change

Checklist

Additional for Feature type of change

vaadin-bot commented Jun 18, 2024

TC Format Checker Report - 03:56 - 18 - Jun

caalador commented Jun 18, 2024

mcollovati left a comment

Choose a reason for hiding this comment

oliveryasuna commented Jun 19, 2024

mcollovati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollovati Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollovati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollovati commented Jul 1, 2024

mcollovati commented Sep 9, 2024

oliveryasuna commented Sep 9, 2024

Added support to customizable the `projectFileExtensions`. #19550

Added support to customizable the `projectFileExtensions`. #19550

Additional for `Feature` type of change

mcollovati Jul 1, 2024 •

edited

Loading