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

WIP feat(StaticViewStrategy): ability to statically analyze aurelia modules #606

Closed
wants to merge 1 commit into from

Conversation

bigopon
Copy link
Member

@bigopon bigopon commented Mar 25, 2018

This PR adds static view strategy, which will give ability to declare all resources used in a view statically, also optionally lazily.

Syntax:

@staticView({
  // the template of this custom element, can be omitted if custom attribute
  template: HTMLTemplateElement | null | undefined;

  // the resources that will be used inside the view
  resources: Function[];

  // address of the view, in case there is some dynamic loading involved
  moduleId?: string;

  // used to declare lazy dependencies, mainly for code splitting purposes
  imports?: () => Promise<Record<string, Function>>;
})

Usage example:

import { CellService } from 'cell-service';
import { staticView } from './static-view';
import { h } from './dom-util';
import view from './calendar.html';

@staticView({
  template: h(
    'template',
    { 'click.delegate': 'click()' },
    'Cell ${message}'
  ),
  resources: []
})
class Cell {
  count = 1;
  message = `Cell ${this.count}`;

  click() {
    this.message = `Cell ${++this.count}`;
  }
}

@staticView({
  template: view,
  resources: [Cell],
  imports: () => import('./test-cell')
})
export class Calendar {

  static inject = [CellService];

  constructor(service) {
    this.service = service;
  }

  load() {
    this.isLoaded = true
  }

  searchAll() {
    this.service.searchAllCells();
  }
}

Differences compared to dynamic loading:

  • @staticView doesn't use Loader to load resources, as everything is declared statically, and thus, bundler friendly.
  • @staticView can use local class as custom elements, custom attributes, unlike dynamic module loading, where classes need to be exported to be discovered. (As shown in the example above, Cell class is declared locally, and only used within Calendar view)
  • @staticView can take pre-built HTMLTemplateElement as its template config, thus help avoid issue with IE Html parser, where it strip invalid markup, (e.g: <template/> element inside a <table/> element). This also helps user to use hyper script syntax to pre built the template, and theoretically JSX too (for fun purposes I guess)
  • @staticView has config property imports, that return a promise, which can be used to declared code split point via () => import('my-module') . This is shown via imports: () => import('./test-cell') in above example, though you need to go to the demo in following link to check

Demo

https://stackblitz.com/edit/statically-aurelia?file=calendar.js

Demo with jsx

https://stackblitz.com/edit/statically-aurelia-zwqj67?file=calendar.js

Further improvement:

  • aurelia-dialog already accepts a class as ViewModel in its config, so it doesn't need any tweaks. That said, we can provide an alternative version, where we only accepts classes as view model, and remove the usage of dynamically loaded view model via module string, to help tree shaking @StrahilKazlachev

  • aurelia-router should follow aurelia-dialog and accept a class directly at moduleId config. @davismj

With the two points above checked, we can build statically analyzable Aurelia apps without any special plugin / configuration. Thus should be friendly to modern bundler.

@EisenbergEffect @jdanyow @jods4
@jsobell

TODO

  • test
  • improve API
  • docs on usage, pros / cons

@bigopon bigopon changed the title feat(StaticViewStrategy): ability to statically analyze aurelia modules WIP feat(StaticViewStrategy): ability to statically analyze aurelia modules Mar 25, 2018
@Alexander-Taran
Copy link

But dynamic compose is not a viable case for this right?
Just to be clear

@davismj
Copy link
Member

davismj commented Mar 25, 2018

Questions and comments:

This should also accept a string, right? template: HTMLTemplateElement | string | null | undefined;

// address of the view, in case there is some dynamic loading involved

Dunno what that means without an example.

@staticView can use local class as custom elements, custom attributes, unlike dynamic module loading, where classes need to be exported to be discovered. (As shown in the example above, Cell class is declared locally, and only used within Calendar view)

Why is that a benefit?

@staticView can take pre-built HTMLTemplateElement as its template config, thus help avoid issue with IE Html parser, where it strip invalid markup, (e.g: element inside a element).

Very cool.

aurelia-router should follow aurelia-dialog and accept a class directly at moduleId config

Agreed! It's something on my 2.0 radar from talks with @jods4. Additionally, router should accept a function for the moduleId that returns a class or a string for code splitting. Is there an issue for this already that you know of? Otherwise I'll create and we can associate it here.

@bigopon
Copy link
Member Author

bigopon commented Mar 25, 2018

@Alexander-Taran

But dynamic compose is not a viable case for this right?

<compose/> config viewModel, works in the same way with aurelia-dialog, so we can still apply this, except that instead of string for module Id, it will be the custom element class:

<template>
  <compose view-model.bind='vm' ></compose>
</template>
export class MyElement {
  viewModels = {
    date: DateEditor,
    number: NumberEditor
  }

  // change this to another class if needed
  vm = this.viewModels.date;
}

This should also accept a string, right? template: HTMLTemplateElement | string | null | undefined;

Yes, it accepts string, HTMLTemplateElement for normal view, null / undefined for @noView

Why is that a benefit?

It's easier to scope local resources used in a view, thus aid app structuring / prototyping

Agreed! It's something on my 2.0 radar from talks with @jods4. Additionally, router should accept a function for the moduleId that returns a class or a string for code splitting. Is there an issue for this already that you know of? Otherwise I'll create and we can associate it here.

you formatted the md wrong 😄 @davismj

@bigopon
Copy link
Member Author

bigopon commented Mar 25, 2018

Agreed! It's something on my 2.0 radar from talks with @jods4. Additionally, router should accept a function for the moduleId that returns a class or a string for code splitting. Is there an issue for this already that you know of? Otherwise I'll create and we can associate it here.

I forgot to answer your question. I don't think there would be any issue, consider the fact that router only care about the first custom element it hits in the module anyway, which means giving it a class or a module should be no different. But I haven't dug into the router code enough to confirm this.

@StrahilKazlachev
Copy link
Contributor

@bigopon

  1. No <compose> and aurelia-dialog do not accept the same config options.
    • <compose> - viewModel: string | object - so either a moduleId or an object to be used as the VM.
    • aurelia-dialog - accepting and a class is possible since the DialogService uses Origin.get to convert it to moduleId: string. This part is causing the most issues with webpack, ask @jods4.
    • both are built on top of the CompositionEngine.
  2. In aurelia-router the RouteLoader is an abstract class that must be provided for the router. The one provided by the templating-router is also on top of the CompositionEngine.
  3. The dialog APIs may seem ready for supporting this case, but the internals are all based on dynamic composition.

@bigopon
Copy link
Member Author

bigopon commented Mar 25, 2018

With the help of aurelia/framework#858, probably we can make it easier by allowing config to opt out the default resources that aren't used, then we can make <compose/> v2, targeting the same name to support static scenario. Something like this

typeof NO_AU_COMPOSE_V1 === 'undefined' ? Compose : ComposeV2

@obedm503
Copy link

Really cool. Why still take a moduleId? This would mean Aurelia still needs a loader specific to the bundler used. Once this is introduced I doubt there would be a need for using moduleIds, or is there?

@bigopon
Copy link
Member Author

bigopon commented Mar 25, 2018

Really cool. Why still take a moduleId? This would mean Aurelia still needs a loader specific to the bundler used. Once this is introduced I doubt there would be a need for using moduleIds, or is there?

I was thinking of supporting <require/> inside the markup, which will be converted to pure import. If it's hard to do, we can just scrap it. And no, module Id has no association with loader, it's acts as a base url.

@bigopon
Copy link
Member Author

bigopon commented Mar 28, 2018

On a second thought, a decorator seems to be an overkill, probably we can make it cleaner, by enabling convention via static property on the class:

import view from './calendar.html'

class Calendar {

  static view = {
    template: view,
    resources: [],
    imports: () => import('splitted-bundle.js')
  }

  static inject = [CellService];

  constructor(service) {
    this.service = service;
  }

  load() {
    this.isLoaded = true
  }

  searchAll() {
    this.service.searchAllCells();
  }
}

@obedm503
Copy link

A decorator seems better because it aligns itself with the rest of the API. What makes it seem like an overkill?

@StrahilKazlachev
Copy link
Contributor

We can have both, like with inject.

@EisenbergEffect
Copy link
Contributor

Instead of it being @staticView, I think we should just call it @view. Also, what about renaming resources to dependencies. I'm thinking of making that name change in vNext (since it's all container-based) and was wondering if this new API could be forward compatible. Finally, is there any way to combine the imports property with dependencies?

@bigopon
Copy link
Member Author

bigopon commented Apr 5, 2018

Renaming it to @view would be very nice, as if needed, we can make it behave like @inject and static inject = [...]

about dependencies/imports, probably we can unify everything into 1:

@view({
  template: template,
  dependencies: () => [...] // let devs decide what to go in here, more flexibility
})
class myclass {

}

@EisenbergEffect
Copy link
Contributor

Sounds good. Let's go for that and see how it turns out.

@obedm503
Copy link

obedm503 commented Apr 5, 2018

can dependencies just be an array also? when it comes to local dependencies, most of the time they're not going to be dynamically imported

@bigopon
Copy link
Member Author

bigopon commented Apr 6, 2018

yeah, we could have that. final API could look like

interface IViewConfig {
  template: string | HTMLTemplateElement | Promise<string | HTMLTemplateElement>;
  dependencies?: Function[] | { (): Function[] | Promise<Function[]> };
}

@bigopon bigopon mentioned this pull request Apr 14, 2018
1 task
@EisenbergEffect
Copy link
Contributor

@bigopon I've merged #614. Can we close this now?

@bigopon
Copy link
Member Author

bigopon commented Apr 18, 2018

closed as this is part of #614

@bigopon bigopon closed this Apr 18, 2018
@bigopon bigopon deleted the static-aurelia branch April 18, 2018 06:19
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.

6 participants