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

feat(config): ability to declare global resources by classes #858

Closed
wants to merge 2 commits into from

Conversation

bigopon
Copy link
Member

@bigopon bigopon commented Feb 4, 2018

At the moment, Aurelia only absorbs global resource declarations via modules loading, which also enables all sorts of lazy loading: component level, router level, but there is no convenient way to load resources directly (or synchronously) via implementation (classes). This PR enables that scenario, which should also help bundler bundle Aurelia application more effectively.

4 new APIs added to FrameworkConfiguration:

  • customElement(implementation: Function): void
  • customAttribute(implementation: Function): void
  • bindingBehavior(implementation: Function): void
  • valueConverter(implementation: Function): void

Examples:

Current way

/**
 * @param {FrameworkConfiguration} config
 */
export function configure(config) {
  config.globalResources(
    './my-module',
    './my-resources'
  );
}

new way

import { MyElement, MyAttribute } from './my-module';
import { MyBindingBehavior, MyValueConverter } from './my-resources'

/**
 * @param {FrameworkConfiguration} config
 */
export function configure(config) {
  config.customElement(MyElement);
  config.customAttribute(MyAttribute);
  config.bindingBehavior(MyBindingBehavior);
  config.valueConverter(MyValueConverter);
}

How this helps: we see that classes / implementations are used directly and thus, should be more friendly with bundlers. An example would be tree shaking: if we are to use module name via string, there would be no way to tree shake an unused resources, but if we are to use classes, it would be possible. Following example is how aurelia-templating-resources would look like if this was supported:

Current way

  function configure(config) {
  injectAureliaHideStyleAtHead();

  config.globalResources(
    PLATFORM.moduleName('./compose'),
    PLATFORM.moduleName('./if'),
    PLATFORM.moduleName('./else'),
    // ...
  );

  // ....

}

New way

import { Compose } from './compose'
import { If } from './if'
import { Else } from './else'

function configure(config) {
  injectAureliaHideStyleAtHead();

  config.customElement(Compose);
  config.customAttribute(If);
  config.customAttribute(Else);

  // ...
}

So it can be tree shake via environment variables, with the help of minifiers like the following examples:

New way + environment variables + tree shaking

import { Compose } from './compose'
import { If } from './if'
import { Else } from './else'

function configure(config) {
  injectAureliaHideStyleAtHead();

  if (typeof AU_NO_COMPOSE === 'undefined') {
    config.customElement(Compose);
  }
  if (typeof AU_NO_IF === 'undefined') {
    config.customAttribute(If);
    config.customAttribute(Else);
  }
  
  // ...
}

And then in bundlers, we can easily shake default resources via defining the environment variables. For example, say we know our application won't use compose, just need to do like following in webpack configuration:

  new webpack.DefinePlugin({
    AU_NO_COMPOSE: true
  });

With this enabled, we can offer more out of the box binding behaviors/ value converters with fool proof tree shaking configuration and bundle size will never go off the roof.


Caveats:

  • Loading custom elements won't be easy because this breaks the convention: view url has to be either specified explicitly or import & inlined into view model classes. However, this could be easier once we introduce single file component.
  • Only works for global resources

Summary:

pros

  • Bundler friendlier
  • Tree shake-able resources
  • perf (as they are all synchronous declaration), no need for creating promiose to load many resources, and modules can be easily concatenated (scope hoisting)

cons

  • Convention of view finding based on view model is not applicable.
  • ??

API

The api could be improved, so it doens't have to be too tedious:

From:

  config.customElement(A);
  config.customElement(B);
  // ...

To:

  config.global({
    customElements: [A, B, C],
    customAttributes: [D, E, F],
    bindingBehaviors: [I, J, K],
    valueConverters: [X, Y, Z]
  });

Update 1:

I have overloaded the method globalResources. Now it can take either string as module name, class (function) as implementation or one object for explicit configuration. Example:

  1. Explicit
// Note that class names are treated as is when using this overload
config.globalResources({
  customElements: [Compose],
  customAttributes: [If],
  bindingBehaviors: [Self], // <=== SelfBindingBehavior will not be converted to self
  valueConverters: [SanitizeHTML] // <=== SanitizeHTMLValueConverter will not be converted
})
  1. Implicit, via implementation
config.globalResources(
  If,  // <-- will rely on customAttribute('if') call
  Else,
  // ....
  Compose, // <-- will rely on customElement('compose') call
  SelfBindingBehavior, // <-- will be converted based on convention
  OneTimeBindingBehavior, // <-- will be converted based on convention
  // ....
  SanitizeHTMLValueConverter // <-- will be converted based on convention
);
  1. Mixed
config.globalResources(
  If,  // <-- will rely on customAttribute('if') call
  PLATFORM.moduleName('./else'), // <-- like the old way
  // ....
);

It needs to be mentioned explicitly somewhere in the doc that loading resources via module name string will always come after the loading resources via implementation, as one is loaded during POST task, and the other is loaded during PLUGIN(MAIN) task


Update 2: Some details for discussion

  • Should resources be loaded in order ? Which means, either loading resources via string as module name or directly as implementation will have their order respected. This requires changes in ViewEngine.importViewResources:
  config.globalResources(
    './my-module',
    // Should it respect the order and only load MyClass
    // after all resources in my modules have been loaded ?
    MyClass, 
  )

How resources are loaded currently with the new API: synchronous resources will be loaded first, if it is a custom element, its view will be loaded after all resources that is loaded via module name have been loaded..

  • Named export in webpack is normally not mangled, but class name will be (I think this is configurable, but on new version only). How should we document this, as MyBindingBehavior class will not be registered as my binding behavior when building for production using the new API.

@bigopon
Copy link
Member Author

bigopon commented Feb 4, 2018

@EisenbergEffect @jdanyow

@AshleyGrant
Copy link
Collaborator

Couldn't we just have config.component(Foo)?

@bigopon
Copy link
Member Author

bigopon commented Feb 4, 2018

You meant component instead of customElement ?

If so I would say we alias it to customElement, as component doesnt go well with customAttribute

import { metadata } from 'aurelia-metadata';


/** @typedef ConfigInfo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen this notation before. How is this different from declaring an interface?

Copy link
Member Author

@bigopon bigopon Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think interface won't work in the mixed environment so I used jsdoc instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the jsdoc notation is similar to interface, except that it could work without any issues in a JS environment. Once we switch to TS, it can simply be changed to interface

@AshleyGrant
Copy link
Collaborator

No, I just mean have component, as the class itself (or a decorator on the class) already tells Aurelia what it is. Custom Elements, Custom Attributes, Value Converters, Binding Behaviors, View Engine Hooks, etc are all generically referred to as components.

So, why should the proposed API require a developer write redundant code that tells the framework that the custom attribute is a custom attribute (as an example)?

Another thought would be to just overload the globalResources function with this new capability.

@bigopon
Copy link
Member Author

bigopon commented Feb 4, 2018

Yes, that's what I wanted to do, but it needs some discussion. I would prefer to have it right at globalResources but currently we are throwing error when any of the parameters in globalResources is not a string, thus making it work with non string would be a breaking change. If not, then I think globalResources would be the best API

About relying on the class name, I was a bit in @jods4 side while doing it, as it doesn't go well with minifier or bundler that mangle named export. But there is no reason to not support that.

@AshleyGrant
Copy link
Collaborator

Class names being mangled by a minifier isn't much of a problem. Just create Babel/TypeScript plugins that automatically add the metadata at compile time 😀 Then the developer had the ear of using a naming convention, with the size savings that come with a minifier.

@bigopon
Copy link
Member Author

bigopon commented Feb 4, 2018

Maybe we can treat it the way we currently treat it, find explicit configuration that was added via decorators, then convention all the way and fallback to custom elements. Then it's devs' job to ensure that a value converter isn't used as a custom element 😄

@AshleyGrant
Copy link
Collaborator

I'm fine with that. I'm also fine with keeping the function names you have currently. I just want to have component or resource added.

@bigopon
Copy link
Member Author

bigopon commented Feb 4, 2018

I have updated the PR, globalResources is now overloaded with 2 extra signatures. #858 (comment)

@jsobell
Copy link
Contributor

jsobell commented Feb 4, 2018

I like this a lot!
We'll have to make sure the need for developers to use PLATFORM.moduleName for WebPack view module reference is well documented (until we find a better way), but this is an awesome backwards-compatible improvement, and tidies up the whole global thing nicely

@bigopon bigopon changed the title feat(config): ability to declare global resources by classes WIP feat(config): ability to declare global resources by classes Feb 5, 2018
@veikkoeeva
Copy link

veikkoeeva commented Feb 11, 2018

It looks to me @bigopon has written text that could pass partly as documentation. So I think if documentation should be amended also and if so, which pieces should be amended?

@jods4
Copy link
Contributor

jods4 commented Feb 13, 2018

Some thoughts.

One nice thing about this API is that it moves away from module names embedded in strings, which is not friendly with modern JS tools. Adopting this provides a path to get rid of PLATFORM.moduleName.

As far as registering multiple components, what about config.customElement(A, B, C)?

One limitation of global resources is that they can't be lazy-loaded.
An API such as config.customElement("my-data-grid", () => import("my/data-grid")) might help us achieve that (more design required).

Removing modules as string has been discussed a few times previously and the most important problem that has not been adressed here is how the "origin" concept and relative sub-resources are loaded by aurelia-loader.

@bigopon
Copy link
Member Author

bigopon commented Feb 13, 2018

I was thinking about global resources lazy loading, but couldn't find a good reason why a global resources needs to be both declared global, and lazy loaded, as by the time application starts, all global resources need to be ready. Otherwise, they could have been in local view resources hierarchy.

Origin was something I was unable to address and wanted a discussion for. It feels hard to ensure origin when resources are registered this way. Origin won't be completely lost though, as they take the origin of the resources index, which is the file containing configure method. In a hoisting bundler environment, is there a way to achieve Origin, or is Origin needed in that way ? On the other hand, origin concept was to ensure the convention works, so this API can be distributed with a warning: convention not applicable, use @inlineView, @useView(view-url) instead.

@bigopon
Copy link
Member Author

bigopon commented Feb 13, 2018

As far as registering multiple components, what about config.customElement(A, B, C)?

It's a nice API, config.globalResources(A, B, C) does the same thing, regardless custom element, attribute, value converter, binding behavior, view engine hooks... but the method could take another overload.

@EisenbergEffect
Copy link
Contributor

@bigopon Great work. Just let me know when you feel it's ready.

@jods4
Copy link
Contributor

jods4 commented Feb 20, 2018

couldn't find a good reason why a global resources needs to be both declared global, and lazy loaded, as by the time application starts, all global resources need to be ready.

(Emphasis mine) This is how it works today. If we add a lazily loaded API such as suggested config.customElement("my-data-grid", () => import("my/data-grid")) then Aurelia should be modified to not load this component until it first sees <my-data-grid> in a view.

Today this is achieved with local resources, but if you use a large component on a handful of pages, it becomes a bit annoying and registering it globally is simpler.

On the other hand, origin concept was to ensure the convention works, so this API can be distributed with a warning: convention not applicable, use @inlineView, @useView(view-url) instead.

Beware, here be dragons. I would really like if we moved away from the origin concept and I am totally OK with saying that when you use these new API you can't rely on conventions.

That said, origin sneaks up in unexpected places at times. I remember we had quite a few problems with aurelia-dialog which needs to find the origin of ViewModels, which are typically provided from an import rather than a string... To that effect, I recall there's very ugly code that iterates all loaded Webpack modules (with a dependency on internal implementation of webpack runtime) looking for the exported class, hoping to "forge" an origin.

Basically what I'm saying is: yes let's drop origin when working we new apis but be prepared for some lib fixing.

In a hoisting bundler environment, is there a way to achieve Origin

aurelia-webpack-plugin prevents hoisting modules loaded by aurelia-loader.

@bigopon
Copy link
Member Author

bigopon commented Mar 6, 2018

@jods4 This API benefits plugin distribution more than app development. Normally, in an application, I wouldn't want to mess with file path (read Origin), while for plugin, I can't care less about it, and all I want is to packed things into one file to distribute easily. Probably it's @EisenbergEffect 's call

@obedm503
Copy link

I'm really happy for this pr as it directs aurelia to be more bundler/loader agnostic 🕺

@EisenbergEffect
Copy link
Contributor

Ok, I want to do a final review of this, but I'd like to get this in soon. Assuming we have the warning that this registration method doesn't work with conventions, are there any objections to us merging this and pushing it out in a release (perhaps next week)?

@obedm503
Copy link

quick question, will this pr allow us to declare local dependencies by passing the class directly to @inlineView instead of Array<{ from: string, as: string }>?

@inlineView('<template>cool content</template>')
@customElement('custom-element')
class CustomElement {}

const view = `
  <template>
    cool content <custom-element></custom-element>
  </template>
`;
@inlineView(view, [CustomElement])
class App {}

@bigopon
Copy link
Member Author

bigopon commented Mar 24, 2018

@obedm503 theoretically yes. It could be phase 2 of this feature, where we can do things you just suggested, and also something like following (though I'm not sure, need some experiments)

// custom-element.js
@inlineView('<template>cool content</template>')
@customElement('custom-element')
class CustomElement {}

// app.js
const view = `
  <template>
    cool content <custom-element></custom-element>
  </template>
`;
@inlineView(view, () => import('custom-element.js'))
class App {}

@bigopon
Copy link
Member Author

bigopon commented Mar 24, 2018

@obedm503 Update to the above, the loading resources locally has very little to do with this PR, but it's what I'm thinking of. Sorry for the misinformation.

@bigopon bigopon changed the title WIP feat(config): ability to declare global resources by classes feat(config): ability to declare global resources by classes Mar 24, 2018
@stalniy
Copy link

stalniy commented May 13, 2018

guys, when do you plan to merge this?

@EisenbergEffect
Copy link
Contributor

@stalniy We may not get this exact PR in, but we will get this feature in. After reviewing this PR, we started to go back and take these ideas and push them down into the core of the framework. @bigopon has put together a number of PRs to different parts of the framework to make this even more official, and not just something that is patched up on the top layer. We've merged a number of those PRs already and I believe he's got one or two more coming that will finish off the new feature set.

@JSeligsohn
Copy link

@bigopon @EisenbergEffect
I wanted to check in as to the current state of these changes. What is supported as of today? Is there documentation outlining how one could achieve similar things to the original scenario described?

@bigopon bigopon deleted the aurelia-sync branch December 22, 2020 13:28
@bigopon
Copy link
Member Author

bigopon commented Dec 22, 2020

@JSeligsohn the documentation of this feature isn't in yet ...

With the latest version of the core modules, instead of the following:

aurelia.use.globalResources([
  'my-element',
  'my-attribute'
])

you can do:

import { MyCustomElement } from './my-element'
import { MyCustomAttribute } from './my-attribute'

aurelia.use.globalResources(
  MyCustomElement,
  MyCustomAttribute,
)

For custom elements that are registered via their constructors, their views need to be on the static property $view, so it looks like this:

class MyCustomElement {
  static $view = '<template>${message}</template>'
}

If external resources need to be imported into a view, use a property dependencies to configure. This is the equivalent of <require from="..."> in traditional template. And the static $view property will need to be turned into an object, instead of a simple string. Like following example:

class MyCustomElement {
  static $view = {
    template: '<template>${message}</template>',
    dependencies: [MyAttribute, () => import('my-module').then(myModule => myModule.MyClass)]
  }
}

Another feature is the ability to configure resources without having to import anything from Aurelia core modules, this is to make it simpler to develop a plugin/share code. To do this, use a static property $resource on the constructor of either a custom element, custom attribute, value converter, binding behavior or view engine hook to declare their resource type, name, and various configuration.

Example for a custom element:

class MyCustomElement {
  static $resource = {
    name: 'my-el', // equivalent of @customElement('my-el')
    type: 'element', // can be omitted if element
    processContent: () => {...}, // equivalent of @processContent(...)
  }
}

or custom attribute:

class MyCustomAttribute {
  static $resource = {
    type: 'attribute',
    name: 'my-attr',
    // etc...
  }
}

@JSeligsohn
Copy link

@bigopon Thank you! This is very helpful.
Just a clarification question - you said For custom elements that are registered via their constructors..., is that always the case when registering a custom element via global resources? What's the other way they're registered?

@bigopon
Copy link
Member Author

bigopon commented Dec 23, 2020

@JSeligsohn you can see from this example:

class MyCustomElement {
  static $view = {
    template: '<template>${message}</template>',
    dependencies: [
      MyAttribute
    ]
  }
}

It can also be like this

class MyCustomElement {
  static $view = {
    template: '<template>${message}</template>',
    dependencies: [
      MyAttribute,
      MyAnotherCustomElement
    ]
  }
}

@JSeligsohn
Copy link

@bigopon Are you saying the MyAnotherCustomElement would only be registered in that case by its constructor? Whereas MyCustomElement itself could/would be registered in a different way?

@bigopon
Copy link
Member Author

bigopon commented Dec 23, 2020

@JSeligsohn I was answering this Q:

is that always the case when registering a custom element via global resources? What's the other way they're registered?

Basically there' 2 types of resources: global, and local. Both types can be registered using either constructor or string for module path. For the above example, I was giving an example of how to register a custom element, with constructor reference MyAnotherCustomElement, locally to MyCustomElement

@JSeligsohn
Copy link

@JSeligsohn I was answering this Q:

is that always the case when registering a custom element via global resources? What's the other way they're registered?

Basically there' 2 types of resources: global, and local. Both types can be registered using either constructor or string for module path. For the above example, I was giving an example of how to register a custom element, with constructor reference MyAnotherCustomElement, locally to MyCustomElement

Oh, okay, I see now what you mean. Thanks for the clarification!

@JSeligsohn
Copy link

Tangentially related, @bigopon, are you aware of a way to actually create a custom element "on the fly"? Let's say I dynamically built an html "view" as well as a "viewModel" (class), could I then put those two together and make it into a custom element at runtime? The additional challenge here seems to me to be that aurelia can only reference "modules" that were already built at compile time, but no way to pass in a runtime created class. I welcome any suggestions.

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 this pull request may close these issues.

9 participants