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

InlineViewDependenciesPlugin doesn't suport 2nd and 3rd arg #140

Open
3cp opened this issue Mar 14, 2018 · 7 comments
Open

InlineViewDependenciesPlugin doesn't suport 2nd and 3rd arg #140

3cp opened this issue Mar 14, 2018 · 7 comments

Comments

@3cp
Copy link
Member

3cp commented Mar 14, 2018

@jods4, the current implementation assumes inlineView only takes 1 arg. But inlineView api supports additional 2nd and 3rd args. Given user suppose to use PLATFORM.moduleName on 2nd arg. It should be an easy fix for this line to length >= 1 && length <= 3. I am too lazy to fork and create PR.

if (expr.arguments.length !== 1)

http://aurelia.io/docs/api/templating/function/inlineView

inlineView(markup: string, dependencies?: Array, dependencyBaseUrl?: string): any

Just point out, I also has suspicion about 3rd arg dependencyBaseUrl, it looks like complicating situation of PLATFORM.moduleName in 2rd arg.

@3cp 3cp changed the title InlineViewDependenciesPlugin doesn't suport InlineViewDependenciesPlugin doesn't suport 2nd amd 3rd arg Mar 16, 2018
@3cp 3cp changed the title InlineViewDependenciesPlugin doesn't suport 2nd amd 3rd arg InlineViewDependenciesPlugin doesn't suport 2nd and 3rd arg Mar 16, 2018
@jods4
Copy link
Contributor

jods4 commented Mar 16, 2018

Seems reasonable, PR welcome.

Two questions, though:

  1. Should we make moduleName optional in the second argument as inlineView is enough of a marker to detect the module names? Or maybe not for the sake of consistency? I'm leaning towards consistency: less code, less edge cases, less documentations, less surprises for users.

  2. What does dependencyBaseUrl do exactly? Does it impact the paths in dependencies? If yes, I suppose we either need to process it some way, or forbid its use with Webpack (is that even reasonable? If so, the error message should suggest an alternative, what would it be?)

@3cp
Copy link
Member Author

3cp commented Mar 19, 2018

Absolutely agree on your first question.

On second:

Both noView and inlineView have this similar api. I can think about two type of usages.

  1. @noView(['a.css'], 'base/url');, this looks unnecessary.
  2. @noView(['a.css'], 'http://cdn.com/path');, this looks like the designed usage.

But astonishingly, apps built with cli + requirejs/systemjs/webpack totally ignored base url, no matter using PLATFORM.moduleName or not on dependency.

So, the api doc lies, baseUrl doesn't work 😆

I guess it might worked long long time ago with jspm based app, plus aurelia-loader implementation at that time.

Theoretically, both requirejs and systemjs have no problem to support loading resource from absolute url. But apparently I guess latest loader-default has some missing part to turn on requirejs/systemjs on this feature.

Correct me if I am wrong, I am not webpack user, from my limited understand in webpack, I don't think its module loader can support loading resource from absolute url.

Which means you don't need to do anything extra, rather than fixing the length check on inlineView. At least for now.

@jods4
Copy link
Contributor

jods4 commented Mar 19, 2018

Interesting, thanks for clarifying all that.

The behavior I would expect is that base + uri are combined before being passed to the loader... Never used this feature before, not sure if it's a long standing bug, or if nobody uses it (anymore?).

The way module ids are handled is 100% dependent on the loader.
As far as Webpack goes, it expects opaque module ids that should be inside its built bundles.

In other words, you are correct about webpack: it is not really a loader, only a bundler. aurelia-loader-webpack does not know how to load an external module from an http url.

To enable that kind of scenarios, one would need to create a composite Aurelia loader... For example one that uses import(uri) for modules starting with http:// and delegates to aurelia-loader-webpack for the rest.

@3cp
Copy link
Member Author

3cp commented Mar 19, 2018

Right, the module id is resolved by inlineView implementation when baseUrl is present. Somewhere is broken before module id was finalized.

It seems like a broken api feature need to be removed. This would not be a broken change, as Aurelia has policy to against. Because this feature does not work, nobody has code rely on it.

@bigopon
Copy link
Member

bigopon commented Nov 25, 2019

@3cp well that means if we want ux style to be loaded similarly to user app styles, then theres no way but to let the convention kick in. The issue with that is we cant have nice plugin build process where it create a single dist file instead of bunch of dist files. If its the only way then ill revert this build config changes. @EisenbergEffect is it not acceptable to make user import the style independently?

@3cp
Copy link
Member Author

3cp commented Nov 25, 2019

You can load inline css string through aurelia api, like aurelia-dialog did.

@bigopon
Copy link
Member

bigopon commented Nov 25, 2019

Loading css the dialog way isolates the style from user css pipeline. I think ill revert ux build for now

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

No branches or pull requests

3 participants