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

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

Jump to bottom

Open

tomtomau wants to merge 5 commits into symfony:main

from tomtomau:aurelia

Open

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

tomtomau wants to merge 5 commits into symfony:main from tomtomau:aurelia

+421 −5

Conversation

@tomtomau

Copy link

@tomtomau tomtomau commented Jan 25, 2018

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

@tomtomau


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

9b5cc67

weaverryan

weaverryan requested changes

Jan 30, 2018

View reviewed changes

index.js

/**

* If enabled, the Aurelia plugin is enabled

Copy link

Member

@weaverryan weaverryan Jan 30, 2018

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

@tomtomau tomtomau Feb 3, 2018

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

@weaverryan weaverryan Jan 30, 2018

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

"homepage": "https://github.com/symfony/webpack-encore",

"dependencies": {

"aurelia-loader-webpack": "^2.1.0",

Copy link

Member

@weaverryan weaverryan Jan 30, 2018

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)

index.js

return this;

configureResolveModules(directories) {

Copy link

Member

@weaverryan weaverryan Jan 30, 2018

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

@tomtomau tomtomau Feb 3, 2018

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.

@tomtomau


 fixes from PR review

785e8ae

Lyrkan

Lyrkan requested changes

Jan 31, 2018

View reviewed changes

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

lib/WebpackConfig.js Outdated

this.vueLoaderOptionsCallback = vueLoaderOptionsCallback;

}

enableAureliaPlugin(aureliaPluginConfig = {}) {

Copy link

Collaborator

@Lyrkan Lyrkan Jan 31, 2018

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

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

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

Yep - my bad!

lib/config-generator.js Outdated

config.resolve.alias['react-dom'] = 'preact-compat';

}

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

Copy link

Collaborator

@Lyrkan Lyrkan Jan 31, 2018

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

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

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

Done

lib/plugins/aurelia.js Outdated

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

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

Copy link

Collaborator

@Lyrkan Lyrkan Jan 31, 2018

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

@Lyrkan Lyrkan Jan 31, 2018

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;

package.json

"yargs": "^8.0.1"

"devDependencies": {

"aurelia-webpack-plugin": "^2.0.0-rc.5",

Copy link

Collaborator

@Lyrkan Lyrkan Jan 31, 2018

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

@tomtomau tomtomau Feb 3, 2018

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

@tomtomau tomtomau Feb 3, 2018

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

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

...

On 2018年1月31日 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> .

@Lyrkan Lyrkan added the Feature label

Feb 1, 2018

tomtomau added 3 commits

February 3, 2018 12:47

@tomtomau


 Fixes from PR

fbed96d

@tomtomau


 Write tests for Aurelia and fix ensurePackagesExist call

7285fdd

@tomtomau


 Add tests for the configureResolveModules option

720abcd

@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

Labels

Feature

3 participants

@tomtomau @weaverryan @Lyrkan

Uh oh!

Uh oh!

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 tomtomau commented Jan 25, 2018

Uh oh!

@weaverryan weaverryan Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

@weaverryan weaverryan Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

@weaverryan weaverryan Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

@weaverryan weaverryan Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

Uh oh!

@Lyrkan Lyrkan Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

@Lyrkan Lyrkan Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

@Lyrkan Lyrkan Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

@Lyrkan Lyrkan Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

@Lyrkan Lyrkan Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

@tomtomau tomtomau Feb 3, 2018

Choose a reason for hiding this comment

Uh oh!

tomtomau commented Jan 31, 2018 via email

Uh oh!

tomtomau commented Feb 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants