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

Sub-dependencies of bundle dependencies are going into wrong bundle #342

Closed
pfurini opened this issue Sep 27, 2016 · 37 comments · Fixed by #541
Closed

Sub-dependencies of bundle dependencies are going into wrong bundle #342

pfurini opened this issue Sep 27, 2016 · 37 comments · Fixed by #541
Labels

Comments

@pfurini
Copy link
Contributor

pfurini commented Sep 27, 2016

I'm submitting a bug report

  • Library Version:
    0.20.0

Please tell us about your environment:

  • Operating System:
    OSX 10.11.6

  • Node Version:
    6.4.0

  • NPM Version:
    3.10.8

  • Browser:
    all

  • Language:
    TypeScript 2.0

Current behavior:
When you have the standard bundling config in aurelia.json (one vendor-bundle and one app-bundle), only the root file of listed dependencies goes into the vendor-bundle, while the rest of the package files goes into the app-bundle.
Here is an example of bundles node in aurelia.json file:

    "bundles": [
      {
        "name": "app-bundle.js",
        "source": [
          "[**/*.js]",
          "**/*.{css,html}"
        ]
      },
      {
        "name": "vendor-bundle.js",
        "prepend": [
          "node_modules/bluebird/js/browser/bluebird.core.js",
          "scripts/require.js"
        ],
        "dependencies": [
          "whatwg-fetch",
          "aurelia-binding",
          "aurelia-bootstrapper",
          "aurelia-dependency-injection",
          "aurelia-event-aggregator",
          "aurelia-framework",
          "aurelia-history",
          "aurelia-history-browser",
          "aurelia-loader",
          "aurelia-loader-default",
          "aurelia-logging",
          "aurelia-logging-console",
          "aurelia-metadata",
          "aurelia-pal",
          "aurelia-pal-browser",
          "aurelia-path",
          "aurelia-polyfills",
          "aurelia-route-recognizer",
          "aurelia-router",
          "aurelia-task-queue",
          "aurelia-templating",
          "aurelia-templating-binding",
          "extend",
          {
            "name": "aurelia-fetch-client",
            "path": "../node_modules/aurelia-fetch-client/dist/amd",
            "main": "aurelia-fetch-client"
          },
          {
            "name": "aurelia-api",
            "path": "../node_modules/aurelia-api/dist/amd",
            "main": "aurelia-api"
          },
          {
            "name": "jwt-decode",
            "path": "../node_modules/jwt-decode/lib",
            "main": "index"
          },
          {
            "name": "aurelia-authentication",
            "path": "../node_modules/aurelia-authentication/dist/amd",
            "main": "aurelia-authentication"
          },
          {
            "name": "text",
            "path": "../scripts/text"
          },
          {
            "name": "aurelia-templating-resources",
            "path": "../node_modules/aurelia-templating-resources/dist/amd",
            "main": "aurelia-templating-resources"
          },
          {
            "name": "aurelia-templating-router",
            "path": "../node_modules/aurelia-templating-router/dist/amd",
            "main": "aurelia-templating-router"
          },
          {
            "name": "aurelia-testing",
            "path": "../node_modules/aurelia-testing/dist/amd",
            "main": "aurelia-testing",
            "env": "dev"
          }
        ]
      }
    ]

Take for example the aurelia-authentication dependency: only the main file (aurelia-authentication.js) goes into the vendor-bundle, while the dependent files goes into app-bundle..

Expected/desired behavior:

  • What is the expected behavior?
    Every sub-dependency of a dependency in the vendor-bundle should go into the vendor-bundle and not into the app-bundle.
  • What is the motivation / use case for changing the behavior?
    The current behavior defeats the purpose of having separate bundles, especially when the app-bundle gets polluted with library (not user) code.
@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

I want to add that, because of this bug, also the generated source.map is technically wrong (I mean based on the specification found here ).
The sources section lists library files (the same files that got included in the app-bundle), that are not present in the sourcesContent section, and some browsers (e.g. Firefox) report this as an error due to a malformed or incomplete map file.

ADDENDUM
Here is what I found at the end of the sourcesContent array (at the end of the map file):

"sourcesContent":[<omitted real source code>,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null],"sourceRoot":"../src"]

You can see that, in place of the source code of library files, you have null values, and that's a specification error..

@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

I've done some more investigation, and it seems that aurelia-cli is not doing dependency analysis on the specified main file.. Correct me if wrong..

Trying to run the dependency-tree library on a requirejs main file gives the correct dependency tree, as shown in the following output after a simple run against the aurelia-authentication.js file (in the package's dist/amd folder):

{
    "/Users/paolof/Developer/reginacarmeli/irc-web/client/node_modules/aurelia-authentication/dist/amd/aurelia-authentication.js": {
        "/Users/paolof/Developer/reginacarmeli/irc-web/client/node_modules/aurelia-authentication/dist/amd/authFilterValueConverter.js": {},
        "/Users/paolof/Developer/reginacarmeli/irc-web/client/node_modules/aurelia-authentication/dist/amd/authenticatedValueConverter.js": {
            "/Users/paolof/Developer/reginacarmeli/irc-web/client/node_modules/aurelia-authentication/dist/amd/aurelia-authentication.js": {}
        },
        "/Users/paolof/Developer/reginacarmeli/irc-web/client/node_modules/aurelia-authentication/dist/amd/authenticatedFilterValueConverter.js": {
            "/Users/paolof/Developer/reginacarmeli/irc-web/client/node_modules/aurelia-authentication/dist/amd/aurelia-authentication.js": {}
        }
    }
}

This output could be easily mapped to DependencyInclusion/SourceInclusion objects and correctly added to the final bundle..

@AStoker
Copy link
Contributor

AStoker commented Sep 27, 2016

@Nexbit, this definitely seems like it shouldn't be happening. Can you reproduce this by creating a new project from scratch with the CLI? I have not experienced this behavior on any of my CLI projects.

@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

Sure, I'll try asap! Did you find something looking wrong in my dependencies section? Just to be sure I'm not missing something obvious ..

Il giorno 27 set 2016, alle ore 15:00, Andrew Stoker [email protected] ha scritto:

@Nexbit, this definitely seems like it shouldn't be happening. Can you reproduce this by creating a new project from scratch with the CLI? I have not experienced this behavior on any of my CLI projects.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@AStoker
Copy link
Contributor

AStoker commented Sep 27, 2016

Thanks. And no, the bundles config looks normal, as far as I can tell.
On a different note, you might also try upgrade your Node to version 6.6.0.

@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

Hi
I can confirm the bug persists doing the following:

  • au new project from scratch, with typescript, stylus preprocessor (it doesn't matter anyway), testing support
  • NO automatic npm install
  • Changed aurelia-cli version from 0.18.0 to 0.20.0 (that's really odd, please fix it soon!)
  • The complete package.json with all the dependencies is as follows:
{
  "name": "cli-test1",
  "description": "An Aurelia client application.",
  "version": "0.1.0",
  "repository": {
    "type": "???",
    "url": "???"
  },
  "license": "MIT",
  "dependencies": {
      "aurelia-animator-css": "^1.0.0",
      "aurelia-api": "^3.0.0-rc9",
      "aurelia-authentication": "^3.0.0-rc11",
      "aurelia-bootstrapper": "^1.0.0",
      "aurelia-cookie": "^1.0.9",
      "aurelia-dialog": "^1.0.0-beta.3.0.0",
      "aurelia-fetch-client": "^1.0.0",
      "aurelia-notify": "^0.6.0",
      "bluebird": "^3.4.6",
      "jwt-decode": "^2.1.0",
      "lodash": "^4.16.1",
      "moment": "^2.15.1",
      "rest": "^2.0.0",
      "whatwg-fetch": "^1.0.0"
  },
  "peerDependencies": {},
  "devDependencies": {
    "aurelia-cli": "^0.20.0",
    "aurelia-testing": "^1.0.0-beta.2.0.0",
    "aurelia-tools": "^0.2.2",
    "browser-sync": "^2.13.0",
    "connect-history-api-fallback": "^1.2.0",
    "gulp": "github:gulpjs/gulp#4.0",
    "gulp-changed-in-place": "^2.0.3",
    "gulp-plumber": "^1.1.0",
    "gulp-rename": "^1.2.2",
    "gulp-sourcemaps": "^2.0.0-alpha",
    "gulp-notify": "^2.2.0",
    "minimatch": "^3.0.2",
    "through2": "^2.0.1",
    "uglify-js": "^2.6.3",
    "vinyl-fs": "^2.4.3",
    "event-stream": "^3.3.3",
    "gulp-typescript": "^2.13.6",
    "gulp-tslint": "^5.0.0",
    "tslint": "^3.11.0",
    "typescript": "^2.0.0",
    "typings": "^1.3.0",
    "gulp-stylus": "^2.5.0",
    "jasmine-core": "^2.4.1",
    "karma": "^0.13.22",
    "karma-chrome-launcher": "^1.0.1",
    "karma-jasmine": "^1.0.2",
    "karma-typescript-preprocessor": "^0.2.1"
  }
}
  • Done npm install successfully
  • Installed a couple of bower packages as well (bower.json follows):
{
  "name": "cli-test1",
  "description": "",
  "main": "",
  "license": "ISC",
  "homepage": "",
  "private": true,
  "ignore": [
    "**/.*",
    "node_modules",
    "bower_components",
    "test",
    "tests"
  ],
  "dependencies": {
    "device.js": "devicejs#^0.2.7",
    "fastclick": "^1.0.6"
  }
}
  • Copied the following aurelia.json:
{
  "name": "cli-test1",
  "type": "project:application",
  "platform": {
    "id": "web",
    "displayName": "Web",
    "output": "bundles",
    "index": "index.html"
  },
  "transpiler": {
    "id": "typescript",
    "displayName": "TypeScript",
    "fileExtension": ".ts",
    "dtsSource": [
      "./typings/**/*.d.ts",
      "./custom_typings/**/*.d.ts"
    ],
    "source": "src/**/*.ts"
  },
  "markupProcessor": {
    "id": "none",
    "displayName": "None",
    "fileExtension": ".html",
    "source": "src/**/*.html"
  },
  "cssProcessor": {
    "id": "stylus",
    "displayName": "Stylus",
    "fileExtension": ".styl",
    "source": "src/**/*.styl"
  },
  "editor": {
    "id": "vscode",
    "displayName": "Visual Studio Code"
  },
  "unitTestRunner": {
    "id": "karma",
    "displayName": "Karma",
    "source": "test/unit/**/*.ts"
  },
  "paths": {
    "root": "src",
    "resources": "src/resources",
    "elements": "src/resources/elements",
    "attributes": "src/resources/attributes",
    "valueConverters": "src/resources/value-converters",
    "bindingBehaviors": "src/resources/binding-behaviors"
  },
  "testFramework": {
    "id": "jasmine",
    "displayName": "Jasmine"
  },
  "build": {
    "targets": [
      {
        "id": "web",
        "displayName": "Web",
        "output": "bundles",
        "index": "index.html"
      }
    ],
    "loader": {
      "type": "require",
      "configTarget": "vendor-bundle.js",
      "includeBundleMetadataInConfig": "auto",
      "plugins": [
        {
          "name": "text",
          "extensions": [
            ".html",
            ".css"
          ],
          "stub": true
        }
      ]
    },
    "options": {
      "minify": "stage & prod",
      "sourcemaps": "dev & stage"
    },
    "bundles": [
      {
        "name": "app-bundle.js",
        "source": [
          "[**/*.js]",
          "**/*.{css,html}"
        ]
      },
      {
        "name": "vendor-bundle.js",
        "prepend": [
          "node_modules/bluebird/js/browser/bluebird.core.js",
          "scripts/require.js"
        ],
        "dependencies": [
            "whatwg-fetch",
          "aurelia-binding",
          "aurelia-bootstrapper",
          "aurelia-dependency-injection",
          "aurelia-event-aggregator",
          "aurelia-framework",
          "aurelia-history",
          "aurelia-history-browser",
          "aurelia-loader",
          "aurelia-loader-default",
          "aurelia-logging",
          "aurelia-logging-console",
          "aurelia-metadata",
          "aurelia-pal",
          "aurelia-pal-browser",
          "aurelia-path",
          "aurelia-polyfills",
          "aurelia-route-recognizer",
          "aurelia-router",
          "aurelia-task-queue",
          "aurelia-templating",
          "aurelia-templating-binding",
          "extend",
          {
            "name": "aurelia-fetch-client",
            "path": "../node_modules/aurelia-fetch-client/dist/amd",
            "main": "aurelia-fetch-client"
          },
          {
            "name": "aurelia-api",
            "path": "../node_modules/aurelia-api/dist/amd",
            "main": "aurelia-api"
          },
          {
            "name": "lodash",
            "path": "../node_modules/lodash",
            "main": "lodash.min"
          },
          {
            "name": "moment",
            "path": "../node_modules/moment/min",
            "main": "moment.min"
          },
          {
            "name": "aurelia-dialog",
            "path": "../node_modules/aurelia-dialog/dist/amd",
            "main": "aurelia-dialog",
            "resources": [
              "../../styles/output.css"
            ]
          },
          {
            "name": "aurelia-notify",
            "path": "../node_modules/aurelia-notify/dist/amd",
            "main": "aurelia-notify",
            "resources": [
              "style.css",
              "bs-notification.html"
            ]
          },
          {
            "name": "aurelia-cookie",
            "path": "../node_modules/aurelia-cookie/dist/amd",
            "main": "cookie"
          },
          {
            "name": "jwt-decode",
            "path": "../node_modules/jwt-decode/lib",
            "main": "index"
          },
          {
            "name": "aurelia-authentication",
            "path": "../node_modules/aurelia-authentication/dist/amd",
            "main": "aurelia-authentication"
          },
          {
            "name": "text",
            "path": "../scripts/text"
          },
          {
            "name": "aurelia-templating-resources",
            "path": "../node_modules/aurelia-templating-resources/dist/amd",
            "main": "aurelia-templating-resources"
          },
          {
            "name": "aurelia-templating-router",
            "path": "../node_modules/aurelia-templating-router/dist/amd",
            "main": "aurelia-templating-router"
          },
          {
            "name": "aurelia-testing",
            "path": "../node_modules/aurelia-testing/dist/amd",
            "main": "aurelia-testing",
            "env": "dev"
          }
        ]
      }
    ]
  }
}

If I run au build --dev env now, the bundles look correct.
As soon as I simply COPY over my src folder into the test project, and run the build again, the aforementioned issue appears.
The generated bundles are exactly the same I found in my original project, so I'm confident that the issue relies in the current cli version.

IMHO the issue is related to the analysis of source files' dependencies, that obviously are too many to post here..
I can confirm what I mentioned in my original post: not all the library files are put in the app-bundle, but only the dependant files of some dependencies, while the main one remains in the vendor-bundle.

If you want to give me a hint on where to look for the src files analysis in the cli sources, maybe I could find a way to debug it on my project.

Thx, P.

@AStoker
Copy link
Contributor

AStoker commented Sep 27, 2016

First thing, I don't believe --dev env is a proper flag. I think you want --env dev, but since the development environment is default, you don't need that flag at all.
I'm trying to figure out exactly what you're doing here, as the default project doesn't put anything in the app-bundle other than the app code. So I think you got something going on here that's hosing your setup. Creating a new project with the cli and building it puts no vendor code into my app bundle.
Do this for me if you could. Create a clean CLI project (feel free to update the package for the cli to version 0.20.0). Install dependencies, and then build the project. Does any vendor code end up in the app bundle?
Don't copy your old aurelia.json file when testing, because that's probably the culprit.
Can you tell me specifically what vendor code is getting into your app bundle? If you're pulling in bower components, you might need to add those to the vendor bundle.

@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

First thing, I don't believe --dev env is a proper flag. I think you want --env dev, but since the development environment is default, you don't need that flag at all.

You're right, it was a mistake in my post. The command I run was au build --env dev indeed...

I'm trying to figure out exactly what you're doing here, as the default project doesn't put anything in the app-bundle other than the app code. So I think you got something going on here that's hosing your setup. Creating a new project with the cli and building it puts no vendor code into my app bundle.
Do this for me if you could. Create a clean CLI project (feel free to update the package for the cli to version 0.20.0). Install dependencies, and then build the project. Does any vendor code end up in the app bundle?

Yes, you're right. The steps I mentioned in my previous post were done with the original src content (the empty skeleton generated by au new).
The first test I've done is to create an empty CLI project with the options I mentioned (web, typescript, stylus, with test support). Then I added the dependencies with npm, you can see the exact package.json in my post. Then I run the au build command.
The bundles were correct: a huge vendor-bundle and a tiny app-bundle with the hello world skeleton.

The second test were to open the aurelia.json file generated by the au new command, and put the dependencies that you can find in my above post, that included the couple bower files that I installed using the bower.json file that you also find in my previous post. I DID NOT copy over my aurelia.json file, I edited the new one adding the dependencies...
Then I run the au build command again, and again the bundles were correct: a bigger-than-before vendor-bundle and the same tiny app-bundle that were exact the same as before...

The third test were to go in Finder, and simply copy my src content and paste in the empty CLI project (ripping the old content).
Then I run the au build command and got a lot of TSC compiler errors/warnings that I ignored (no typings installed).
Now the vendor-bundle got smaller, and the app-bundle contained some of the files that were previously in the vendor-bundle. In addition the source map is corrupted as I mentioned in my first post...

The difference between the second and the third test is merely the content of the src folder, hence the conclusion that the bundler gets screwed somewhat by the source code, and that's something that needs investigation IMHO...

@AStoker
Copy link
Contributor

AStoker commented Sep 27, 2016

Okay, thank you for clarifying, I wasn't tracking exactly what you were saying :)
So the question is then, what in your src code that you're copying over to the working skeleton is causing vendor files to be included in the app-bundle?
That is going to help to determine whether or not we have an issue in the CLI, or perhaps some strangeness in the calling code.

@AStoker
Copy link
Contributor

AStoker commented Sep 27, 2016

Also, you say you're pulling in bower modules. Is there a reason you specifically want to pull them down via bower instead of npm? And how are you including them in your package? Are they in your src folder, or in a parent directory and being bundled into something?

@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

So the question is then, what in your src code that you're copying over to the working skeleton is causing vendor files to be included in the app-bundle?

Well.. I don't know, and even if the codebase is not huge, it's not small either! I'm porting over an existing project from jspm to the cli, and I'm half the way in the process.
I noticed the following:

  • Only sub-dependencies of the main file gets bundled in the app-bundle, never the main file itself (for example, the aurelia-authentication dependency gets splitted as follows: aurelia-authentication.js goes into vendor-bundle and the files [authFilterValueConverter, authenticatedValueConverter, authenticatedFilterValueConverter] go into the app-bundle)
  • I tracked down these dependencies, that get into app-bundle (except the main file, as stated above): jwt-decode, aurelia-authentication, aurelia-templating-resources, aurelia-notify
  • Every other dependency except the above ones gets bundled in vendor-bundle

Also, you say you're pulling in bower modules. Is there a reason you specifically want to pull them down via bower instead of npm? And how are you including them in your package? Are they in your src folder, or in a parent directory and being bundled into something?

That doesn't matter for this issue. I tried without them, and nothing changes anyway.
I pull them from bower because they are not correctly published in npm, and I bundle them in vendor-bundle taking them from bower_components dir (see my aurelia.json above), and they work nicely with the rest of the code... No issues here.

That is going to help to determine whether or not we have an issue in the CLI, or perhaps some strangeness in the calling code.

Can you define strangeness from the CLI's point of view?
My code is working fine, no TSLint or tsc errors whatsoever, tests successfully, etc.
If the CLI wants to redefine the concept of correctness, it should be properly documented and in bold typeface.. I don't want a build tool that goes into my way, or I'll switch to something else ASAP... ;)

@pfurini
Copy link
Contributor Author

pfurini commented Sep 27, 2016

I want to add that the packages that got incorrectly bundled in app-bundle (jwt-decode, aurelia-authentication, aurelia-templating-resources, aurelia-notify) are all multi-files dependencies.

A single-file dependency is never put into app-bundle anyway, and it seems coherent with the observation that the main file of a multi-file dependency get always into vendor-bundle, while in my case the dependent files get into the app-bundle.

@EisenbergEffect
Copy link
Contributor

@AStoker Were you able to confirm this behavior? Would you have any bandwidth to look into addressing it?

@AStoker
Copy link
Contributor

AStoker commented Sep 28, 2016

@Nexbit, the CLI shouldn't be defining correctness, didn't mean to come across that way. Just as a disclaimer, the CLI is still in alpha, the price of living on the bleeding edge of technology is that you might bleed a little bit ;)
Clarifying what I meant by strangeness, I mean that there might some kind of strange requests or import statements going on that could be causing code to be improperly bundled. I don't know exactly what kind of "strangeness" it would be, hence why it's strange.

Now, I almost spoke to soon. I was able to reproduce the error, but it was different from yours. We're using the CLI on a large project, pulling in many of the resources you mentioned, and specifically the aurelia-templating-resources. The difference for us is that the aurelia-dialog is the only aurelia package that is sneaking it's way into our app-bundle. Comparing it to packages that don't sneak in (and the ones you mentioned), it seems that packages that we need to define to specifically use AMD are the ones breaking, that is, the packages that are formed like this:

{
    "name": "aurelia-dialog",
    "path": "../node_modules/aurelia-dialog/dist/amd",
    "main": "aurelia-dialog"
}

Now, the dialog module seems to be putting things on the exports in a unique way, and that might be part of the problem there, but it's not the case for the templating resources. So the plot thickens...

I will try to figure out why aurelia dialog is being bundled in the wrong place, and hopefully that will solve your problem @Nexbit. In the mean time, can you send me what your main.js looks like? I'm curious about where/how plugins/files are being imported, and compare that to how I'm doing it.

@EisenbergEffect, I have a little bit of bandwidth, so I'll put some time in, but right now at the office we're trying to solve our issue of testing our application with source code so we can actually do test driven development. I'll have a write up for a proposal I'd like to implement to the CLI to help with this (it will also solve issues with users testing with Wallaby). And don't worry, it's different than what I had initially proposed.

@pfurini
Copy link
Contributor Author

pfurini commented Sep 28, 2016

Comparing it to packages that don't sneak in (and the ones you mentioned), it seems that packages that we need to define to specifically use AMD are the ones breaking

Yeah, I can confirm that, as I stated in my last comment, and the culprit is that they are multi file dependencies.. Single file dependencies never gets bundled in the wrong place.
Can you confirm that, even on your side, the root file (in your case aurelia-dialog.js) gets bundled in vendor-bundle, and the rest of the files (the dependants) gets into one of your app bundles?

Are you bundling some other dependency I mentioned in my list, for example aurelia-authentication? I suspect that could be some sort of circular dependency when you actually import a particular package in your code.
I'll try removing some imports (and eventually stubbing them) one by one and see what happens...

@AStoker
Copy link
Contributor

AStoker commented Sep 28, 2016

I can confirm that the root file is being bundled appropriately.
I haven't bundled aurelia-authentication, wanted to hit it one at a time. I am pulling in aurelia-ui-virtualization and that is a multi file dependency, but it is not being bundled incorrectly. It might be some kind of circular thing, but I made my setup very basic (new cli project, install aurelia-dialog, import it into the app.js).
I'll keep digging over here, and you keep digging over there. Hopefully one of us strikes gold 😄

@AStoker
Copy link
Contributor

AStoker commented Sep 29, 2016

I'm still digging into this (it's kinda kicking my butt right now, since there are still parts of the CLI I'm learning about). A thing of curiosity is that it seems the Amodo Trace function we call in the bundled-source.js seems to trace the dependencies of anything imported in the app.js, and then trace those dependencies (which is all expected).
Theory: When we define the app bundle to import files with the regex **/*.js, it kind of acts as a fallback for putting files in there. The vendor bundle has a list of dependencies that it matches to. However, the dependency of a dependency in the vendor bundle isn't being matched (since it has a different name), and therefore get's put inside the bundle that it does match to (the app bundle with **/*.js).
I'm still digging in, but that's the track I'm going down right now, and hopefully it gives you a hand too @Nexbit if you're looking into it as well.

@pfurini
Copy link
Contributor Author

pfurini commented Sep 29, 2016

@AStoker That's really interesting, good catch! Well, if that's the culprit, maybe the CLI can treat the dependencies of a bundle in a "priority" mode...
I mean, if we prepare a full static dependency tree for each main file of a declared dependency (the ones we list on aurelia.json), we could avoid putting files that we found in that trees when we process files with the Almodo Trace.
To build a quick dependency tree we could use the dependency-tree lib, that based on my previous tests gives accurate results and does not add a significant amount of processing time...

EDIT: only multi-file AMD style dependencies need this special treatment, I suppose

@AStoker
Copy link
Contributor

AStoker commented Sep 29, 2016

I believe this is the area that our troubles are realized (https://github.com/aurelia/cli/blob/master/lib/build/bundled-source.js#L85).
Here, we are tracing the app.js file, and any files that we find that we have not added yet to our bundler are added here, but they are added in such a way that they are "includedBy" the file that was referencing them, in our case the app.js. If the that.includedBy was rather the original referencing file (such as aurelia-dialog.js), then it would be bundled where it is.

@AStoker
Copy link
Contributor

AStoker commented Sep 29, 2016

As a TL;DR of the issue at hand

When you have a vendor library being imported (the vendor library has been defined in the vendor-bundle, and the vendor library has dependent files), the only file that gets correctly placed into the vendor-bundle is the main file of the library itself. Any file dependencies imported from the vendor file are bundled into the app-bundle.

The reasoning for this is that during the bundling process, vendor files are not traced for dependencies, and therefore any file dependencies are not included properly. But when the tracing does begin on the app code, it traces the dependencies of the imported vendor files, and places them inside the app-bundle, since they are not explicitly defined to be in the vendor-bundle.

@sensedeep
Copy link

How can we then write a bundles definition to put all of (say aurelia-dialog) into the vendor bundle?

With this definition as per the doc:

                        "name": "aurelia-dialog",
                        "path": "../node_modules/aurelia-dialog/dist/amd",
                        "main": "aurelia-dialog",
                        "resources": [
                          "../../styles/output.css"
                        ]

we get the following files in the app bundle:

/Users/mob/dev/luis/scripts> grep 'define(' app*.js | sed 's/, .*//' | sort | grep aurelia
define('aurelia-dialog/ai-dialog',['exports'
define('aurelia-dialog/ai-dialog-body',['exports'
define('aurelia-dialog/ai-dialog-footer',['exports'
define('aurelia-dialog/ai-dialog-header',['exports'
define('aurelia-dialog/attach-focus',['exports'
define('aurelia-dialog/dialog-configuration',['exports'
define('aurelia-dialog/dialog-controller',['exports'
define('aurelia-dialog/dialog-options',["exports"]
define('aurelia-dialog/dialog-renderer',['exports'
define('aurelia-dialog/dialog-result',["exports"]
define('aurelia-dialog/dialog-service',['exports'
define('aurelia-dialog/lifecycle',['exports']
define('aurelia-dialog/renderer',['exports']

@pfurini
Copy link
Contributor Author

pfurini commented Oct 4, 2016

@sensedeep Unfortunately you can't as long as this bug is not fixed.. We are still investigating a solution for this.

@sensedeep
Copy link

Thanks. I can help test when you have a fix.

@AStoker
Copy link
Contributor

AStoker commented Oct 4, 2016

I was digging into this, and got as far as I did with my last post. Unfortunately, my priorities got shifted a bit at work, so I'm working on a different feature for test driven development. As of right now, we know the problem... But not the solution. If you're able to help out figure a way to solve this, that would be greatly appreciated @sensedeep

@sensedeep
Copy link

Sorry, but absolutely buried launching SenseDeep. I'd recommend being able to provide a list of files in the aurelia.json bundle group as a work around. Easier than tracing and would 100% solve the issue.

@sensedeep
Copy link

Can I a general question related to this?

Exactly what does Tracing achieve?

The transpiled code has the define declarations added with dependent modules. Assuming that all the source is being compiled and bundled into a file, why not just catenate the source without tracing? When the file is loaded, all define statements will be executed and the dependencies are handled by requirejs. So what does build time tracing achieve? What am I missing?

@EisenbergEffect
Copy link
Contributor

One thing that may not be obvious is that the tracing process also converts the code into a module format that can run in the browser.

You may be right that there could be a way to remove the tracing and just convert the code to the proper format. I'm not sure that would cover all cases though....so I'd be worried about making that change. I'm not sure it would work correctly with dependencies, especially those distributed as multiple modules. eg. aurelia-templating-resources or aurelia-dialog

@sensedeep
Copy link

I can imaging code with a buried require() somewhere in the code that the tracing might pick up.

What is the transformation required to make it run in the browser? It looks like standard AMD declarations.

I was wondering if the compiler babel or tsc could emit the list of modules while compiling somehow. They have the canonical understanding of what modules are being used when they compile the application source -- as ES6 modules are by definition static. I don't have any concrete suggestions, but it just feels like there must be an easier way. I'll see what I can explore.

@EisenbergEffect
Copy link
Contributor

Yes, amd, but they have to be non-anonymous modules. That's part of what the trace process does. Please explore :) We would love to improve this. I wondered if there was a way to replace amodro trace with something like rollup, for example.

@sensedeep
Copy link

Playing with rollup and it looks promising so far. Extremely fast with a good stable of plugins. I can flexibly target any desired module output with a good go-forward bias to ES modules. Via plugins, can parse modules in a wide variety of formats and convert on output.

The question is whether when fully configured it is as complex as what we have now. I'd recommend a good deep look at this as an alternative. I'll keep digging.

@EisenbergEffect
Copy link
Contributor

@sensedeep Thanks for looking into it. I'd love it if we could leverage something like that internally instead of amodro trace. I personally don't know enough to understand if it's a good fit or not. I look forward to hearing more from your research. Thanks for being willing to help.

@suamikim
Copy link

I guess I'm experiencing the same issue which also leads to a poor debug experience of 3rd party dependencies.

What's the current status of this issue? Are there any plans to resolve it in the foreseeable future?

@tdamir
Copy link

tdamir commented Mar 3, 2017

I'm using the 0.25.0 version of aurelia-cli and it works for me when I explicitly define the resources (*.js is also important):

      {
        "name": "aurelia-dialog",
        "path": "../node_modules/aurelia-dialog/dist/amd",
        "main": "aurelia-dialog",
        "resources": [
          "**/*.js",
          "**/*.html",
          "**/*.css"
        ]
      },

@JeroenVinke
Copy link
Collaborator

I have taken a look at this issue and can confirm what's said in this issue.

To reproduce, add a dependency to the vendor-bundle config.:

{
            "name": "my-plugin",
            "main": "my-plugin",
            "path": "../node_modules/my-plugin",
            "resources": []
          }

where the main file of this plugin (my-plugin.js) imports a file (define(["./file1"], function () {});). One other thing needed to reproduce this issue is that you need to import this plugin from app.js: import 'my-plugin';.

What you get is that the ../node_modules/my-plugin/my-plugin (the main file of the plugin) ends up in the vendor-bundle, but ../node_modules/my-plugin/file1.js ends up in the app-bundle.

The difference between file1 and the main file of my-plugin is that the main file gets registered as a Dependency in the CLI when the build process starts, but file1.js is not registered as a Dependency.

When you register file1.js as a resource, file1.js ends up in the correct bundle (the vendor bundle):

{
            "name": "my-plugin",
            "main": "my-plugin",
            "path": "../node_modules/my-plugin",
            "resources": [
               "file1.js
            ]
          }

But it should not be necessary to do this, so ignore it for now.

Why the main file ends up in the vendor-bundle but file1.js won't is because of this check here. When tracing app.js, the main file, which was registered as a Dependency at the start of the build, will not get added to the bundle because it can be found. File1.js however, has not been found and gets added to the app-bundle (here) since the trace is happening for app.js.

The core issue is this: Even though we tell the CLI not to trace (and bundle) dependencies of the source files because of the [] syntax here, when app.js is traced the fileRead callback is still called for all imports of app.js (my-plugin/myplugin.js, my-plugin/file1.js).

This happens because AmodroTrace executes define("app",["exports","my-plugin"]); when it traces app.js, and RequireJS will then try and load my-plugin and subsequently file1.js. Whenever RequireJS wants to load a file, fileRead is called.

@EisenbergEffect what are your thoughts about possible solutions?

@EisenbergEffect
Copy link
Contributor

Ultimately, for dependencies, we want all the code related to that dependency to go into the same bundle that we defined the module entry point in. I'm not sure what the best way to do that technically is. I haven't looked deeply into that code for a while. @JeroenVinke Would you be willing to try to put together a proposal?

@JeroenVinke
Copy link
Collaborator

The fix may be simpler than I thought...

Instead of assuming that this.includedBy is the correct DependencyInclusion, we could look up the DependencyInclusion based on the path of the file that's being included and the path of the each registered dependency.

Current situation
image

Result of change
image

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Mar 14, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants