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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tomtomau
Copy link

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 :)

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

/**
* If enabled, the Aurelia plugin is enabled
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done :)

index.js Outdated
* @param {function} aureliaLoaderOptionsCallback
* @returns {exports}
*/
enableAurelia(aureliaLoaderOptionsCallback = () => {}) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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)

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

configureResolveModules(directories) {
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

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).

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

enableAureliaPlugin(aureliaPluginConfig = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

Yep - my bad!

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

if (this.webpackConfig.resolveModules && Array.isArray(this.webpackConfig.resolveModules)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

Done


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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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;

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

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

@tomtomau
Copy link
Author

tomtomau commented Jan 31, 2018 via email

@Lyrkan Lyrkan added the Feature New Feature label Feb 1, 2018
@tomtomau
Copy link
Author

tomtomau commented Feb 3, 2018

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

@weaverryan weaverryan changed the base branch from master to main November 18, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants