aurelia / webpack-plugin

A plugin for webpack that enables bundling Aurelia applications.
MIT License
90 stars 36 forks source link

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

Open 3cp opened 6 years ago

3cp commented 6 years ago

@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.

https://github.com/aurelia/webpack-plugin/blob/93f7aff96167c7be752a01edd30f2a08c8bb529d/src/InlineViewDependenciesPlugin.ts#L39

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.

jods4 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

bigopon commented 4 years ago

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