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

fix(source-inclusion): detect traceDependencies from dependency resources correctly #525

Closed

Conversation

JeroenVinke
Copy link
Collaborator

#400

There are actually two issues preventing #400 from working. One is fixed by this PR, and the other one seems to be #342.

To reproduce #400:

The setup is as follows. The aurelia-cli-400 app has a dependency called my-dependency which has two files (resource1.js and resource2.js) where resource2.js is a dependency of resource1.js. The my-dependency dependency is defined in the vendor-bundle:

          {
            "name": "my-dependency",
            "path": "../node_modules/my-dependency",
            "main": "index",
            "resources": [
                "[resource1.js]"
            ]
          }

What you'd expect using this setup is that resource1.js ends up in the vendor-bundle but node_modules/my-dependency/resource2.js does not. Without the changes in this PR both resource1.js and resource2.js are in app-bundle.js. This can be explained due to the glob pattern in SourceInclusion being invalid (as mentioned in #400), which causes neither resource1.js or resource2.js to be included in the vendor-bundle. Due to #342 they end up in the app-bundle instead.

With these changes, only resource1.js ends in vendor-bundle, which is what you'd expect. Although #342 is causing resource2.js to be included in app-bundle, so #400 is not completely "fixed" by this PR

@EisenbergEffect
Copy link
Contributor

@JeroenVinke Feel free to merge when you feel it's ready.

@JeroenVinke
Copy link
Collaborator Author

I'm going to take another stab at this and open a new PR

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.

2 participants