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

chore(Ux-Core): switch to rollup, return configure promise #147

Merged
merged 1 commit into from
Apr 3, 2018

Conversation

bigopon
Copy link
Member

@bigopon bigopon commented Feb 2, 2018

  • At the moment all ux modules are distributed as is, which means they are not bundled into a single file for distribution. This could lead to a drop in startup performance where the browser have to deal with many modules, either via webpack / requirejs / systemjs. This PR introduces rollup, in a basic usage, to concatenate all modules of @aurelia-ux/core into a single javascript file for distribution, as this cannot be done easily with only typescript as it supports --outFile for only amd and system module types. Besides this, all css/html distribution configurations could also sit together within one config.rollup.js file, which is a plus.

  • There are also changes in the configure function of @aurelia-ux/core, it now will return a promise that will be resolved once the ux configuration finished, and also cache it. This can be leveraged as ux components will be able to call it repeatedly without worrying about side effect, also helps synchronously load ux custom elements (will give more detail in another PR).

    before, in main.ts

    export function configure(aurelia: Aurelia) {
      aurelia.use.plugin('@aurelia-ux/core');
      aurelia.use.plugin('@aurelia-ux/select/index');
    }

    We see that ux-core is always needed to be mentioned before ux-select, could be beginner unfriendly.


    after, in @aurelia-ux/select/index

    import { configure as uxCoreConfigure } from '@aurelia-ux/core/index';
    
    export function configure(config: FrameworkConfiguration) {
      return uxCoreConfigure(config).then(ux => {
        // ...
      });
    }

    and in main.ts

    export function configure(aurelia: Aurelia) {
      aurelia.use.plugin('@aurelia-ux/select/index');
    }

https://gist.run/?id=7787dea956bffeca3c0bd7c342b69819

The gist above is an example of how a distribution file of ux-core module looks like with this change.

@EisenbergEffect @ZHollingshead


Edit:

Step 2 of this will be ability to distribute each ux component as a single javascript file, with templates inlined to the custom element view model classes. I could already achieved this. But it depends on how this PR goes.

@EisenbergEffect
Copy link
Contributor

@ZHollingshead I'd love it if you could take a look into this. @bigopon has some great ideas here.

@EisenbergEffect
Copy link
Contributor

@bigopon Is this something we should also look at for all the modules we convert to TS for the Aurelia port?

@bigopon
Copy link
Member Author

bigopon commented Feb 2, 2018

@ZHollingShead came up with the single file idea. For the port, i think so

@serifine
Copy link
Contributor

serifine commented Feb 2, 2018 via email

@serifine
Copy link
Contributor

serifine commented Feb 2, 2018

@bigopon @EisenbergEffect
Would we have to pass in a duplicated core ux configuration to each component we wanted to register? Such as disabling the css reset which is determined in the configuration. My worry would be that as the configuration for @aurelia-ux/core grows the overhead from configuring it multiple times would outweigh the boost we would gain elsewhere. Also, will each component run its own instance of UX Core or a shared singleton instance since they are being configured separately?

I had been thinking that if we wanted to make things more beginner friendly, we could the @aurelia-ux/components package to @aurelia-ux/all and let that package also load @aurelia-ux/core in addition to the components. I am not sure the @aurelia-ux/core being after a component would be a big issue even to beginners if we ensure we use good error messages to cover the scenario where they would like to chose specific components rather than using a bundled package.

On the single file front, I think I may have miscommunicated. I was thinking of toying around with something like Universal Module Definition where we could produce a single output that will work across all loaders to decrease build times rather than build four separate distributions. I do however like the idea of the output being put into a single file, will that improve performance? Will it affect bundle sizes?

@bigopon
Copy link
Member Author

bigopon commented Feb 2, 2018

Only the first call will actually do things so performance overhead is non existence. Because of this, it will always be the same instance of UX core.

For the module output, it is fairly easy I believe, but I was following our "tradition", exporting it as different module types. This call will be up to @ZHollingshead and @EisenbergEffect . Doing this could help us to trim the amount of files in distribution folder / files

@serifine
Copy link
Contributor

serifine commented Feb 2, 2018

What about the configuration? UX Core requires a configuration.

If that is in each individual component now, wouldn't you have to pass in the same configuration to each one if it does not use the default config?

I am not sure how I feel about hiding the plugin setup for UX Core inside of components since UX Core also has its own features that it brings to the table. In most scenarios I feel people would take a standard configuration of all components (e.g. @aurelia-ux/all or @aurelia-ux/components) and if we put the UX Core in one of those that would make sense to me as that is a package bundle.

To me, hiding the @aurelia-ux/core setup inside another package feels more confusing, since @aurelia-ux/core is itself its own fully fledged package with features of its own. But that could just be me.

@bigopon
Copy link
Member Author

bigopon commented Feb 2, 2018

The config ux core receives is the same with the config each component receives. So passing it around is safe, You can see it from this line in framework repo https://github.com/aurelia/framework/blob/master/src/framework-configuration.js#L385

Basically all globally applied plugin will have the same first instance of framework configuration as its parameter.

To me, hiding the @aurelia-ux/core setup inside another package feels more confusing, since @aurelia-ux/core is itself its own fully fledged package with features of its own. But that could just be me.

With this PR changes, you can do either

export function configure(aurelia: Aurelia) {
  aurelia.use.plugin('@aurelia-ux/core');
  aurelia.use.plugin('@aurelia-ux/select/index');
}

or just

export function configure(aurelia: Aurelia) {
  aurelia.use.plugin('@aurelia-ux/select/index');
}

There is no difference between them.

@bigopon
Copy link
Member Author

bigopon commented Feb 2, 2018

For the distribution, I think we can distribute it like:

📁dist

  • index_es2015.js
  • index_commonjs.js
  • index_amd.js
  • index_native-modules.js
  • index_umd.js
  • index.d.ts
  • ... other type definitions file

instead of wrapping in folders. @EisenbergEffect @ZHollingshead

@serifine
Copy link
Contributor

If there are no performance hits, then I think this could be useful.

@EisenbergEffect
Copy link
Contributor

@bigopon Is this ready to do then?

@bigopon
Copy link
Member Author

bigopon commented Feb 14, 2018

Let me adjust types, currently it emits types to all distribution, but I think it can be in only one folder types.

📁 dist
    📁 amd
    📁 commonjs
    📁 es2015
    📁 native-modules
    📁 types

thoughts ? @EisenbergEffect @ZHollingshead

@EisenbergEffect
Copy link
Contributor

@bigopon I like this idea. @jdanyow Any opinion?

@bigopon
Copy link
Member Author

bigopon commented Feb 14, 2018

It's ready to use, test locally via

lerna run build --scope @aurelia-ux/core

@EisenbergEffect @ZHollingshead @jdanyow

@bigopon
Copy link
Member Author

bigopon commented Feb 14, 2018

I think the start method could return AureliaUX instance, so in other components package, we can avoid getting it from container. So the configure method of ux-core would be

export declare function configure(
  config: FrameworkConfiguration,
  callback?: (config: AureliaUX) => Promise<any>
): Promise<AureliaUX>;

@EisenbergEffect
Copy link
Contributor

@ZHollingshead Feel free to merge :) No need for my approval.

@Alexander-Taran
Copy link

looking forward to learning all this bundling and distribution ninja-stuff (-:

@fkleuver fkleuver mentioned this pull request Mar 6, 2018
@bigopon
Copy link
Member Author

bigopon commented Mar 22, 2018

@ZHollingshead Good to go now. I also added first test for ux element config.

As aurelia/framework#858 is getting merged soon, we can start rolling every ux element up into 1 file to distribute after this PR gets merged.

@bigopon
Copy link
Member Author

bigopon commented Mar 22, 2018

@ZHollingshead I'm wondering how we can enforce aurelia-binding@^1.7.1 and aurelia-templating-resources@^1.6.0 to ensure compatible with core ?

@serifine
Copy link
Contributor

If we add those as a dependency to the package.json file won't that ensure that we have the correct versions installed?

@bigopon
Copy link
Member Author

bigopon commented Mar 22, 2018

Does that mean that we have to add it to everyone of them or just the root ? This PR doesn't need any more modification in package.json, it can be merged any time. I'm thinking of doing it in another PR where we add rollup config to every element

@serifine
Copy link
Contributor

Given that all the components rely on @aurelia-ux/core I think that adding the references to that one might just be enough.

@bigopon
Copy link
Member Author

bigopon commented Mar 22, 2018

👍 @ZHollingshead . This one can be merged. All done here then.

@bigopon
Copy link
Member Author

bigopon commented Mar 22, 2018

Oh actually I was having issue with the test in global style engine. Not sure why. Did it work for you ?

@serifine
Copy link
Contributor

I have not re run the tests on this yet. I am planning to merge after 0.8.0 goes out so we have time to play with the new build setup.

@serifine serifine merged commit 89f5f7f into aurelia:master Apr 3, 2018
@bigopon bigopon deleted the switch-build branch April 5, 2018 01:23
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.

4 participants