webpack-plugin icon indicating copy to clipboard operation
webpack-plugin copied to clipboard

InlineViewDependenciesPlugin doesn't suport 2nd and 3rd arg

Open 3cp opened this issue 7 years ago • 7 comments

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

3cp avatar Mar 14 '18 23:03 3cp

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?)

jods4 avatar Mar 16 '18 17:03 jods4

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.

3cp avatar Mar 19 '18 04:03 3cp

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.

jods4 avatar Mar 19 '18 09:03 jods4

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.

3cp avatar Mar 19 '18 11:03 3cp

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

bigopon avatar Nov 25 '19 23:11 bigopon

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

3cp avatar Nov 25 '19 23:11 3cp

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

bigopon avatar Nov 25 '19 23:11 bigopon