Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use InstallDirectory to locate node tasks instead of the WorkingDirectory #434

Merged
merged 3 commits into from
Feb 28, 2017
Merged

Conversation

Antibrumm
Copy link

This is to support Multi-Module structure where the parent is "just" installing NPM / Node and the child modules are actually calling the build tasks like Grunt / Karma

@Antibrumm
Copy link
Author

ping

@eirslett
Copy link
Owner

Hey, sorry for the late response. I don't fully understand the implications of this change. Can you give an example of how the behavior would differ before/after this patch? Could this be a breaking change for some users?

@Antibrumm
Copy link
Author

No problem. I do not think this should have a big impact in general. People actually using installDir and workDir might need to reconfigure the path, but I think it makes more sense that in this case the installDir is used (as for NPM itself) and not the workingDir.

If desired I can also put a boolean switch to trigger the use of installDir instead of workDir? Like this it would be an opt-in?

I try to show my actual config which runs pretty well (I think) and should show the benefit of a consolidated node_modules location for one complete project:

Structure

.
├── pom.xml                                             (with npm install)
├── package.json                                        (packages used)
├── base
│   ├── core-master
│   │   ├── plenty of modules
│   ├── gruntconfig                                     (plugin configs)
│   │   ├── concat.js
│   │   ├── copy.js
│   │   ├── cssmin.js
│   │   ├── eslint.js
│   │   ├── html2js.js
│   │   └── protractor.js
│   ├── ui-rest
│   │   ├── plenty of modules
│   ├── ui-thymeleaf
│   │   ├── plenty of modules
│   └── ui-angular
│       ├── plenty of modules like the next one
│       └── module 1
│           ├── pom.xml                                 (the actual execution)
│           ├── Gruntfile.js
│           ├── src
│           │   └── test
│           │       ├── conf
│           │       │   └── rules
│           │       ├── e2e                             (e2e tests, runs jetty upfront, only in e2e-test-module)
│           │       │   ├── support
│           │       │   └── tests
│           │       ├── java
│           │       │   └── com
│           │       │       └── ...
│           │       ├── resources
│           │       ├── ut                              (unit test for directives etc)
│           │       │   ├── karma.conf.js
│           │       │   ├── mocks
│           │       │   └── tests
│           │       │       ├── datePicker
│           │       │       ├── file-attachments
│           │       │       └── logger
│           │       └── webapp
│           │           └── WEB-INF
│           │               └── spring
│           └── target
├── etc
├── node
│   └── node_modules
│       └── npm
├── node_modules
├── non-repo-libs
├── src
│   └── main
│       └── assembly
└── target

POM Toplevel

<node-version>v6.2.1</node-version>
<npm-version>3.9.5</npm-version>

<plugin>
    <groupId>com.github.eirslett</groupId>
    <artifactId>frontend-maven-plugin</artifactId>
    <version>${frontend-plugin-version}</version>
    <inherited>false</inherited>
    <configuration>
        <nodeVersion>${node-version}</nodeVersion>
        <npmVersion>${npm-version}</npmVersion>
        <installDirectory>./</installDirectory>
        <srcdir>.</srcdir>
    </configuration>
    <executions>

        <!-- 1. Install node and npm locally -->
        <execution>
            <id>install node and npm</id>
            <goals>
                <goal>install-node-and-npm</goal>
            </goals>
            <configuration>
            </configuration>
        </execution>

        <!-- 2. Install node and npm plugins (package.json) -->
        <execution>
            <id>npm install</id>
            <goals>
                <goal>npm</goal>
            </goals>
            <!-- Optional configuration which provides for running any npm command -->
            <configuration>
                <arguments>install</arguments>
            </configuration>
        </execution>
    </executions>
</plugin>

POM Module 1

<plugin>
    <groupId>com.github.eirslett</groupId>
    <artifactId>frontend-maven-plugin</artifactId>
    <version>${frontend-plugin-version}</version>
    <configuration>
        <nodeVersion>${node-version}</nodeVersion>
        <npmVersion>${npm-version}</npmVersion>
        <installDirectory>../../../</installDirectory>
        <srcdir>.</srcdir>
    </configuration>
    <executions>

        <execution>
            <id>install node and npm</id>
            <goals>
                <goal>install-node-and-npm</goal>
            </goals>
            <configuration>
                <skip>true</skip>
            </configuration>
        </execution>

        <execution>
            <id>npm install</id>
            <goals>
                <goal>npm</goal>
            </goals>
            <configuration>
                <skip>true</skip>
            </configuration>
        </execution>

        <!-- 3. Run grunt for generate resources -->
        <execution>
            <id>grunt process-resources</id>
            <goals>
                <goal>grunt</goal>
            </goals>
            <phase>process-resources</phase>
            <configuration>
                <arguments>default</arguments>
                <triggerfiles>
                    <triggerfile>avoidToSpeedUpEclipse</triggerfile>
                </triggerfiles>
                <skip>${skipTests}</skip>
            </configuration>
        </execution>

        <!-- 4. Run karma tests -->
        <execution>
            <id>javascript tests</id>
            <goals>
                <goal>karma</goal>
            </goals>
            <phase>test</phase>
            <configuration>
                <karmaConfPath>src/test/ut/karma.conf.ci.js</karmaConfPath>
                <skip>${skipTests}</skip>
            </configuration>
        </execution>
    </executions>
</plugin>

karma.conf.js

module.exports = function(config) {
    config.set({
        basePath : '../../..',
        frameworks : [ 'jasmine' ],
        files : [ 'target/generated-web-resources-tmp/META-INF/resources/webjars/jquery/2.2.4/jquery.js',
                '...', 
                'target/generated-web-resources-tmp/resources/**/*.js',
                // templates joined
                'target/generated-web-resources-tmp/templates.*.js',
                'src/test/ut/mocks/**/*.js', 
                'src/test/ut/tests/**/*.js' ],
        reporters : [ 'progress' ],
        port : 9876,
        logLevel : config.LOG_INFO,
        browsers : [ 'PhantomJS' ],
        singleRun : false,
        autoWatch : false,
        plugins : [ 'karma-jasmine', 'karma-phantomjs-launcher' ]
    });
};

@jgangemi
Copy link
Contributor

jgangemi commented Aug 5, 2016

👍 would like to see some form of this get released asap.

@eirslett
Copy link
Owner

eirslett commented Aug 6, 2016

I'm sorry I've been so slow at reviewing this! The issue is about optimisation, right? About not having to litter the project directory with node_moduels everywhere? I had a proper look, and I have some concerns. First of all, this change will be a breaking change, so it would need a 2.0 release of the plugin, and break people's setup in unexpected ways. But I don't like the thought of adding a boolean flag either - it's one more configuration flag to keep track of, and there are already too many.

Arguably this could be seen as a problem with the npm ecosystem itself rather than with this maven plugin - in Java we have .m2 which caches the dependencies, and they are share between modules, so that you don't have the extra overhead of having jar files spread around everywhere. While npm installs things locally everywhere.

Have you tried using ied for this purpose? It's a drop-in replacement for npm, inspired by ~/.m2 layout and the nix package manager. It symlinks packages instead of copying them everywhere, which should solve the problem you're trying to solve.

@Antibrumm
Copy link
Author

In the end its all about locating the initial command files, like the grunt script itself. Once within grunt the resolution seems fine as all node/npm tasks are found correctly.

I see several possibilities to achieve that:

  • use workingdir (as originally implemented)
  • use installdir (as in this patch)
  • use a mix of both above. (File.exists?)
  • use a boolean to decide which approach to use
  • allowing explicit declaration of execution script location, like the grunt script location, on the specific maven task.

For me the most logical is using the installdir, as thats what installdir means. But i understand that this might break existing projects. ( all tests are stable atm)

What do you think?

@eirslett
Copy link
Owner

eirslett commented Aug 6, 2016

Ok, I see, it's all about the location of Gruntfile etc? And you want the gruntfiles to be shared amongst all modules?
In that case I would suggest something like adding a separate Gruntfile for every submodule, and then require() the main Gruntfile from there?

@eirslett
Copy link
Owner

eirslett commented Aug 6, 2016

You can also do something like "npm run-script grunt", then in package.json in the submodule you have an npm script which calls "node ../../node_modules/grunt/bin/grunt xxx". And then use the npm mojo.

@Antibrumm
Copy link
Author

Erm no. The problem is the "hardcoded" location of the grunt task bash script in the class DefaultGruntRunner. If we could give an explicit location for this inside the maven plugin I could live with it :)
Basically the DefaultGruntRunner and the other implementations use the workingdir to locate the bash script. This is what the pull request "fixes", it uses the installdir for these locations.

@eirslett
Copy link
Owner

eirslett commented Aug 6, 2016

I see what you mean. Make GruntRunner at all was a mistake, and it really should be removed. (We only need to keep the install mojo and the npm mojo) The best way to run builds in node/npm in general is to simple use npm scripts (and the npm mojo with this plugin). That means you have full flexibility to do whatever you want, including giving an explicit location for every task to run. It's really a lot like using exec-maven-plugin, except you have "node" and "npm" on your path. I would consider GruntRunner more like a handy preset that works in most cases, than a catch-all go-to for every case, in which something more low-level like the npm mojo is needed.

@Antibrumm
Copy link
Author

Hey there. I have given it another try, but I would really like to avoid using yet-another-frontend-plugin.
I don't see why making GruntRunner was a mistake :) and I would love to avoid the exec-maven-plugin for the sake of not having to configure node/npm path explicitly, your plugin just works very nice!

I have now added a little File.exists to support the original implementation of using the WorkingDir and only if it does not find the bash script it tries the InstallDir. With this approach the feature should not be a breaking change anymore.

We are currently using a own hosted plugin version from my fork and I would love to get rid of that so that I can follow your updates easier. Could you take another quick look? All the tests are running well from what I see.

If you still don't like to add the Pull Request please reject this one so that i can bury my hopes :)

@eirslett
Copy link
Owner

That looks a lot better! Then it's also backwards compatible. Thanks!

@Antibrumm
Copy link
Author

Hey Eirslett
I can confirm that version 1.4 is now working with a project structure mentioned in this comment: #434 (comment)

Thanks!

pgerber added a commit to tocco/frontend-maven-plugin that referenced this pull request Apr 25, 2017
This reverts commit db2e984, reversing
changes made to e568deb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants