feat(aurelia): Add an Aurelia plugin option and expose module resolve #248

tomtomau · 2018-01-25T11:24:31Z

I thought Symfony needed some Aurelia love :)

I'm fairly new to Webpack so any feedback is welcomed

As Aurelia leverages dynamic modules, exposing the module resolve configuration let's the Aurelia Plugin work properly.

Aurelia also uses .html for templating as the standard option, so added in a rule for .html if the Aurelia plugin is enabled.

Will work on an example shortly :)

weaverryan · 2018-01-30T20:03:35Z

index.js

@@ -629,6 +629,18 @@ const publicApi = {
        return this;
    },

+    /**
+     * If enabled, the Aurelia plugin is enabled


Can you add a link to Aurelia here? And what are some valid callback options. We usually like to show an example :)

weaverryan · 2018-01-30T20:03:52Z

index.js

+     * @param {function} aureliaLoaderOptionsCallback
+     * @returns {exports}
+     */
+    enableAurelia(aureliaLoaderOptionsCallback = () => {}) {


If it's a plugin, we try to include that in the name to help be transparent. So, enableAureliaPlugin

weaverryan · 2018-01-30T20:04:58Z

package.json

@@ -26,6 +26,8 @@
  },
  "homepage": "https://github.com/symfony/webpack-encore",
  "dependencies": {
+    "aurelia-loader-webpack": "^2.1.0",


Is this needed? Is this a loader or a plugin?

Also, these should be in devDependencies. The lib/features.js should cause the user to see a nice error that they need to install these (instead of us shipping Encore with them included)

weaverryan · 2018-01-30T20:06:11Z

index.js

@@ -705,6 +717,12 @@ const publicApi = {
        return this;
    },

+    configureResolveModules(directories) {


Is this related to Aurelia? I mean, I know it's a webpack feature, but is it needed? I'm not familiar at all with Aurelia, so you'll have to help us out :)

I'll give my best understanding, again, not super familiar with Webpack.

This is something that's required for Aurelia. Aurelia uses dynamic module loading a fair bit, which means that Webpack doesn't understand how to resolve those dynamic modules. This option then tells Webpack to try resolving via a particular path.

For example, my Webpack config code:

Encore // ... .configureResolveModules([ './assets/app', 'node_modules' ]);

You can read more about it on the aurelia/webpack-plugin Wiki page.

Lyrkan

Hi @tomtomau,

Here are some additional comments.
Some tests should also be added since in its current state I don't think using it as documented would work (because of the callback and require.resolve issues).

Lyrkan · 2018-01-31T09:50:32Z

lib/WebpackConfig.js

@@ -403,6 +405,11 @@ class WebpackConfig {
        this.vueLoaderOptionsCallback = vueLoaderOptionsCallback;
    }

+    enableAureliaPlugin(aureliaPluginConfig = {}) {


In the index.js file that method is called with a callback, not an object.

Usually we expect callbacks there and applies them on the default configs shipped with Encore (you can check configureManifestPlugin() for instance).

Yep - my bad!

Lyrkan · 2018-01-31T09:54:59Z

lib/config-generator.js

@@ -89,6 +91,10 @@ class ConfigGenerator {
            config.resolve.alias['react-dom'] = 'preact-compat';
        }

+        if (this.webpackConfig.resolveModules && Array.isArray(this.webpackConfig.resolveModules)) {


I think this Array.isArray check should be put directly into the configureResolveModules() method of the WebpackConfig.js file and display an error if the given parameter isn't an array (see cleanupOutputBeforeBuild() for instance).

Lyrkan · 2018-01-31T10:02:47Z

lib/plugins/aurelia.js

+
+    loaderFeatures.ensurePackagesExist('aurelia-webpack-plugin');
+
+    const AureliaPlugin = require.resolve('aurelia-webpack-plugin');


Shouldn't that be require('aurelia-webpack-plugin') since a require.resolve() call would return a string and not a constructor?

To be more precise it should probably be something like:

const { AureliaPlugin } = require('aurelia-webpack-plugin');

or without the destructuring assignment:

const AureliaPlugin = require('aurelia-webpack-plugin').AureliaPlugin;

Lyrkan · 2018-01-31T10:03:36Z

package.json

@@ -49,6 +49,7 @@
    "yargs": "^8.0.1"
  },
  "devDependencies": {
+    "aurelia-webpack-plugin": "^2.0.0-rc.5",


I see that's a RC version, is it stable enough (I don't know Aurelia at all)?

IMO, yes. Aurelia has a solid track record of releasing RC's that rarely have many breaking changes. 2.0 of this plugin has not been released but 1.x has been abandoned. They tend to run RC's for a while to be certain that their true x.0 release is reliable after being battle tested a bit :)

When 2.0 is released, I'll do my best to remember to come back and update this though :)

tomtomau · 2018-01-31T12:00:06Z

Thank you for the comments, I will make the changes and update the PR when I have done so :)

…

On Wed, 31 Jan 2018 at 9:17 pm, Vincent Le Biannic ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/plugins/aurelia.js <#248 (comment)> : > +'use strict'; + +const PluginPriorities = require('./plugin-priorities'); +const loaderFeatures = require('../features'); + +/** + * @param {Array} plugins + * @param {WebpackConfig} webpackConfig + * @return {void} + */ +module.exports = function(plugins, webpackConfig) { + if (!webpackConfig.useAurelia) return; + + loaderFeatures.ensurePackagesExist('aurelia-webpack-plugin'); + + const AureliaPlugin = require.resolve('aurelia-webpack-plugin'); To be more precise it should probably be something like: const { AureliaPlugin } = require('aurelia-webpack-plugin'); or without the destructuring assignment: const AureliaPlugin = require('aurelia-webpack-plugin').AureliaPlugin; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#248 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB94G4KQAXqQriN0UKmAAs6f4d82zZRAks5tQEvggaJpZM4Rss-2> .

tomtomau · 2018-02-03T03:25:58Z

Hi @weaverryan and @Lyrkan!

I have updated my pull request per the feedback including adding test cases for both the configureResolveModules call and the enableAureliaPlugin call.

Travis seems to be failing on one of the builds because the aurelia-webpack-plugin dependency is "not published" but I'm not 100% in understanding how this works.

I'm also drafting a blog post walking through how to get Aurelia + Encore setup.

Cheers!
Tom

feat(aurelia): Add an Aurelia plugin option and expose module resolve

9b5cc67

weaverryan requested changes Jan 30, 2018

View reviewed changes

fixes from PR review

785e8ae

Lyrkan requested changes Jan 31, 2018

View reviewed changes

Lyrkan added the Feature New Feature label Feb 1, 2018

tomtomau added 3 commits February 3, 2018 12:47

Fixes from PR

fbed96d

Write tests for Aurelia and fix ensurePackagesExist call

7285fdd

Add tests for the configureResolveModules option

720abcd

weaverryan changed the base branch from master to main November 18, 2020 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aurelia): Add an Aurelia plugin option and expose module resolve #248

feat(aurelia): Add an Aurelia plugin option and expose module resolve #248

tomtomau commented Jan 25, 2018

weaverryan Jan 30, 2018

tomtomau Feb 3, 2018

weaverryan Jan 30, 2018

weaverryan Jan 30, 2018

weaverryan Jan 30, 2018

tomtomau Feb 3, 2018

Lyrkan left a comment

Lyrkan Jan 31, 2018

tomtomau Feb 3, 2018

Lyrkan Jan 31, 2018

tomtomau Feb 3, 2018

Lyrkan Jan 31, 2018

Lyrkan Jan 31, 2018

Lyrkan Jan 31, 2018

tomtomau Feb 3, 2018

tomtomau Feb 3, 2018

tomtomau commented Jan 31, 2018 via email

tomtomau commented Feb 3, 2018


		loaderFeatures.ensurePackagesExist('aurelia-webpack-plugin');

		const AureliaPlugin = require.resolve('aurelia-webpack-plugin');

feat(aurelia): Add an Aurelia plugin option and expose module resolve #248

Are you sure you want to change the base?

feat(aurelia): Add an Aurelia plugin option and expose module resolve #248

Conversation

tomtomau commented Jan 25, 2018

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

Lyrkan 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

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

tomtomau commented Jan 31, 2018 via email

tomtomau commented Feb 3, 2018