Fixed relative path matching issue in ConventionDependenciesPlugin

aurelia / webpack-plugin

A plugin for webpack that enables bundling Aurelia applications.

MIT License

90 stars 36 forks source link

Fixed relative path matching issue in ConventionDependenciesPlugin #192

Closed actuallyrob closed 2 years ago

actuallyrob commented 2 years ago

Fixed issue where minimatch would not match relative paths starting with ../ by feeding it resolved file paths. This caused issues in pnpm monorepos where Aurelia views of plugins were not included in the bundle because because they were wrongly disregarded.

bigopon commented 2 years ago

This fix doesn't look at simple as adding { dot: true } to minimatch call. Can you help with making it easier to verify + collaborate:

use latest master as base?
use a branch instead of master branch (so I can be more comfortable pushing stuff to it)
add a tests to verify the change? (there is an example of a pnpm project, or you can just add the project, I'll take care of the CI config)

actuallyrob commented 2 years ago

use latest master as base I have synced my fork with your latest master before creating the PR. Is that okay?
use a branch instead of master branch You'd like me to open another PR from my fix branch to your master branch? I cannot create a branch in this repo.
add a tests to verify the change Verifying the change is not easy. The errors only exist in run time. I could setup a pnpm monorepo with the aurelia-kendoui-bridge installed to simulate my production environment. But I am not sure how to prove that the required .html files are in my bundle after the change has been made, and not before the change. I'd have to figure that out.

bigopon commented 2 years ago

You'd like me to open another PR from my fix branch to your master branch? I cannot create a branch in this repo.

Yes, another branch other than master in your folk repo. GH allows repo maintainer to push to any folk repo branches, master is fine too, but it's a bit uncomfortable pushing to that branch

Verifying the change is not easy. The errors only exist in run time. I could setup a pnpm monorepo with the aurelia-kendoui-bridge installed to simulate my production environment. But I am not sure how to prove that the required .html files are in my bundle after the change has been made, and not before the change. I'd have to figure that out.

We probably will have to do it the way an existing test is done with karma: it actually runs the bundle and verify if things are run and rendered properly. I guess it's necessary. You don't need to do it until the end, maybe just setup the app, with the simplest reproduction of the issue, and I'll handle the CI update.

bigopon commented 2 years ago

I have synced my fork with your latest master before creating the PR. Is that okay?

Sorry I missed this. I've updated master, and pnpm tests should run better now, please sync with latest master again.

actuallyrob commented 2 years ago

I will sync my fix branch up with master, and create a new PR. Does that sound okay? Or can you edit the source branch of this PR?

bigopon commented 2 years ago

It doesn't seem possible to change the source branch. Only destination can be changed. Let's create a new PR?

actuallyrob commented 2 years ago

Will do!