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

CLI Bundler alias/paths matched incorrectly (Plugin development) #1093

Closed
michaelw85 opened this issue Apr 25, 2019 · 5 comments · Fixed by #1094
Closed

CLI Bundler alias/paths matched incorrectly (Plugin development) #1093

michaelw85 opened this issue Apr 25, 2019 · 5 comments · Fixed by #1094

Comments

@michaelw85
Copy link
Contributor

michaelw85 commented Apr 25, 2019

I'm submitting a bug report

  • Library Version:
    "aurelia-cli: ^1.0.0-beta.15
    Please tell us about your environment:

  • Operating System:
    Windows [10]

  • Node Version:
    v10.15.0

  • NPM Version:
    6.9.0
  • Browser:
    Not browser related

  • Language:
    TS but assume it's an issue for all

  • Loader/bundler:
    CLI Bundler

Current behavior:
When the bundler tries to add aliases it looks at the module id, for example ../src/elements/hello-world and tries to match it with a configured path.
One of the following in a default configuration:

{
  "paths": {
    "root": "dev-app",
    "resources": "../src",
    "elements": "../src/elements",
    "attributes": "../src/attributes",
    "valueConverters": "../src/value-converters",
    "bindingBehaviors": "../src/binding-behaviors"
  }
}

It does this by going over this list top down and will only match 1. For the previously mentioned module id it will match with resources and this will result in failure when running the app.

Logic responsible for this behavior:

// lib/build/bundled-source.js:319
function possibleShortcut(moduleId, paths) {
  const _moduleId = fromDotDot(moduleId);
  for (let i = 0, keys = Object.keys(paths); i < keys.length; i++) {
    let key = keys[i];
    let target = paths[key];
    if (key === 'root') continue;
    if (key === target) continue;

    if (_moduleId.startsWith(target + '/')) {
      return {
        fromId: toDotDot(key + _moduleId.slice(target.length)),
        toId: toDotDot(moduleId)
      };
    }
  }
}
  • What is the expected behavior?
    I would not expect order to influence alias path matching, neither would I expect duplicate entries with different paths to be an issue.
    There is probably a good reason for current approach but I would expect imports to be converted to the actual paths instead of creating a creating a new module that returns an existing one. So search for key and replace by target (path).

A low impact solution might be to search for the best match based on amount of character matched (I think this could be accomplished by sorting by length before looping)
Edit: Bad idea this could break existing setups

  • Steps to reproduce

Given: You have the aurelia cli installed
And: run au new --plugin
And: follow instructions, choose all examples at the final step
And: add import { HelloWorld } from 'elements/hello-world'; to dev-app/app.ts
And add console.log(HelloWorld) to a constructor
And: run au run
When: open localhost:9000 in browser
Then: console will show error

GET http://localhost:9000/src/elements/hello-world.js net::ERR_ABORTED 404 (Not Found)

Expected:
Hello world class should load and be logged in the console.

  • What is the motivation / use case for changing the behavior?
    I would like to utilize paths to keep my code clean and maintainable.

Work around
Reorder paths, deepest first, shortest last. This has a drawback, it will force you to use the correct (deepest) alias.

@3cp
Copy link
Member

3cp commented Apr 25, 2019

TS is different, it requires config in tsconfig.

Here is a fix unreleased.
eeb59c8

Note this fix only config "resources", means you can only do

import { HelloWorld } from 'resources/elements/hello-world';

But not

import { HelloWorld } from 'elements/hello-world';

You might be able to enhance tsconfig to support 'elements/hello-world' but I don't know TS enough.

So for dev-app to access anything in the inner-plugin, use 'resources/...' to load the module. The plugin readme is updated too (unreleased).

Also as you pointed out in lib/build/bundled-source.js, I need to fix the logic to create multiple shortcuts instead of just one.

FYI

"elements": "../src/elements",
"attributes": "../src/attributes",
"valueConverters": "../src/value-converters",
"bindingBehaviors": "../src/binding-behaviors"

These four are not designed to be used in AMD module loading (for both app and plugin projects), although it works because we hijacked requirejs paths config (if I fixed the shortcuts).
They are actually to facilitate au generate command, you can modify those path config to let au generate to generate files in different folder.

@michaelw85
Copy link
Contributor Author

michaelw85 commented Apr 26, 2019

@huochunpeng Thanks for your reply.

This issue is not about typescript. Let me try to explain.
In typescript I can write multiple aliases for all of the path entries. This could be configured like this:

"baseUrl": "src",
"paths": { 
  "resources/*": [ "./" ],
  "elements/*": "./elements",
  "attributes/*": "./attributes",
  "valueConverters/*": "./value-converters",
  "bindingBehaviors/*": "./binding-behaviors"
},

This would make both of the following imports possible from a TS point of view:
import { Foo } from resources/elements/foo
and
import { Foo } from elements/foo

But as you already mentioned possibleShortcut only returns 1 match at the moment so for the previous example only the first will work. It will return the first match based on the order the paths are declared in within the aurelia.json. Currently this will match the resources entry "resources": "../src", because ../src will always match anything within src and it's the first entry.
To illustrate this behavior see this fiddle: https://jsfiddle.net/c3ht51dg/2/

This issue is also not just about the dev-app but also limits the usage of aliases in the inner plugin, this is why I raised this ticket.

If the code would support multiple alias matches this problem would not exist or find the "best" match but I just realized this might break existing setups.

@3cp
Copy link
Member

3cp commented Apr 26, 2019

I acknowledged that shortcuts issue. Will fix that.

@michaelw85
Copy link
Contributor Author

@huochunpeng I have some time to take a look. I will give it a go if you don't mind 😃

@3cp
Copy link
Member

3cp commented Apr 26, 2019

Sure. Thx!

michaelw85 added a commit to michaelw85/cli that referenced this issue Apr 26, 2019
Define multiple aliases when they can be applied for a path.

Closes aurelia#1093
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 a pull request may close this issue.

2 participants