Skip to content
This repository has been archived by the owner on Jan 20, 2019. It is now read-only.

Packaging #110

Merged
merged 16 commits into from Apr 5, 2018
Merged

Packaging #110

merged 16 commits into from Apr 5, 2018

Conversation

twokul
Copy link
Contributor

@twokul twokul commented Sep 12, 2017

@kellyselden
Copy link
Member

With the app.import deprecation, does this RFC try to touch on or solve the problem if importing commonjs?


The current application build process merges and concatenates input broccoli trees. This behaviour is not well documented and is a tribal knowledge. While the simplicity of this approach is nice, it doesn't allow for extension. We can refactor our build process and provide more flexibility when desired.

Most importantly, it helps us achieve:

Choose a reason for hiding this comment

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

The term it is vague in this context, should probably be Strategies or something else

+ Should we introduce the same API on add-on level?
+ Will simple extension strategy “just work”?
+ Communication between strategies

Choose a reason for hiding this comment

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

How would we verify that a given strategy (or set of strategies) produces a valid application output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. We don't validate application output right now (usually build fails and that's how you know that something went wrong). We definitely need to provide an easy way to debug strategies which would help address some of the build issues. My assumption is that a "valid" output would depend on the application itself and suspect we need more experimentation on that front.

Most importantly, it helps us achieve:

+ defining and developing a common language around the subject
+ removing highly coupled code and streamline technical implementation (Ember Engines)
Copy link
Member

Choose a reason for hiding this comment

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

Fastboot is another example of this (see here).

@rwjblue
Copy link
Member

rwjblue commented Sep 12, 2017

@kellyselden:

With the app.import deprecation, does this RFC try to touch on or solve the problem if importing commonjs?

This RFC is not proposing a new deprecation. That mention is under a section titled "Topics for Future RFC's" which is only included to show a bit of future thinking/roadmap for the feature...

@twokul
Copy link
Contributor Author

twokul commented Sep 12, 2017

@kellyselden just to expand on what @rwjblue already said. app.import deprecation requires an RFC on its own so it didn't make sense to go into details as it wasn't the purpose of this RFC.

@kellyselden
Copy link
Member

@twokul Sounds good. I misread the RFC.

Assembler must have the following interface:

```typescript
interface Assembler {

Choose a reason for hiding this comment

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

Is this going to be a public API? So are there use cases for different Assembler implementations? Or is this just for clarification, but the end just an (internal) implementation detail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assembler is more of a concept, than anything else. If you start thinking in terms of strategies, it is nothing more but another type of strategy. As of right now, it is an internal construct.

```typescript
interface Strategy {
constructor(options: any);
toTree(assember: Assembler, inputTree: BroccoliTree): BroccoliTree;

Choose a reason for hiding this comment

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

Curios: are there use cases where a Strategy needs access to the Assembler? Examples don't show this, and given the Assembler interface which only consists of constructor and toTree, looks like this could be omitted/further decoupled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Let's say we want to remove unused modules from an Ember application. Assembler could provide you with a list of modules/files that are used. That way it will be easy for a strategy to produce a broccoli tree with only relevant content. Some pseudocode (not a real implementation):

const concat = require('broccoli-concat');

class DeadModulesEliminationStrategy() {
  constructor(options) {
    this.options = options;
  }

  toTree(assembler, inputTree) {
    return concat(inputTree, {
      // ... code ...
      inputFiles: assembler.getUsedModules()
      // ... code ...
    };
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@twokul it seems odd to add a parameter solely as a bag of potential future API. I had the same question as @simonihmig when reviewing.


```sh
bundler:js:input/
├── addon-modules
Copy link
Member

Choose a reason for hiding this comment

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

This is the directory name? Is is expected that this string would be split or otherwise processed?

(Also, this is not sh syntax afaict)


### Concatenation Strategy

One of the strategies that we already use now to produce `assets/app-name.js` and `assets/vendor.js`. It takes a broccoli tree and returns a new concatenated broccoli tree.
Copy link
Member

Choose a reason for hiding this comment

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

First sentence here needs to be restructured. You might want to make it clear that although you are proposing Strategies as a public API, a similar architecture is already used internally in Ember-CLI. I think that is implied in your statement.


#### Assets Split Strategy

Other strategy that proved very useful is an ability to separate assets. One of the applications would be to separate "static" assets from application code. They are called "static" because they don't change very often and could stay cached by CDN throughout several deployments (`ember.js`, `jQuery` or third party dependencies). That way performance is less effected by deployments.
Copy link
Member

Choose a reason for hiding this comment

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

First sentence needs to be restructured. I think this generally can be clearer.

Copy link
Member

Choose a reason for hiding this comment

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

This example is not just an example, it is how bundling works today. Are you suggesting that it could be ported to a strategy or that it be ported?

Other items in the "examples" section here seem to be only for illustration, but this one I would expect will be written as a built-in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect I need to rewrite this section to be more clear as you suggested. Current bundling works differently (unless I misunderstood what you meant) that is all third party libraries end up in one vendor.js file as opposed to being bundled separately.

I think this strategy could be provided out of the box by Ember CLI.

}
```

Note, that `strategies` is an optional property. If you don't pass it to the constructor or `strategies` is an empty array, you effectively "opting out" of the feature and using the default behaviour.
Copy link
Member

Choose a reason for hiding this comment

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

you => you're

# Drawbacks

- More libraries to maintain
- Increasing public API surface
Copy link
Member

Choose a reason for hiding this comment

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

I encourage you to think through this section more instead of only providing it lip service. Some ideas to start with:

  • Does this feature further couple Ember apps and addons to Broccoli? Is that acceptable or a risk we can mitigate?
  • defaultStrategies is an array in your example above. Is it ok that someone could push onto that array? Should we consider freezing the object or instead exporting a function that generates a new array on each invocation?
  • Are there existing addons dependent on the current bundling implementation in Ember-CLI?
  • Are there build performance concerns?
  • Are the files being bundled only JavaScript, for example post HBS compilation? How can we bundle intelligently when we have a mix of HBS and JS files, or is that concern handled elsewhere?

I see you have some of these under unresolved, maybe some eventually will become drawbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all valid points and @rwjblue and I discussed all of them. having said that, we definitely need to have them in writing. let me address that.


# Alternatives

[Webpack](https://webpack.js.org) has become very popular in solving this similar problem. However it is unclear how WebPack could be used with Ember as Ember has both explicit and implicit dependencies. This is why we feel we need something that is aware of how Ember apps are assembled and take advantage of existing tools when it is makes sense. Since we have an opinionated way of structuring our apps, we can avoid the "wall of configuration" that other asset packaging systems are susceptible to.
Copy link
Member

Choose a reason for hiding this comment

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

it is unclear how WebPack could be used with Ember as Ember has both explicit and implicit dependencies.

Can you provide examples?

we feel we need something that is aware of how Ember apps are assembled

You've gone into some effort in this RFC to make clear that strategies are agnostic bundling steps. I don't see how they are coupled to how Ember apps are assembed, can you provide an example?

@Fed03
Copy link

Fed03 commented Sep 13, 2017

This behaviour is not well documented and is a tribal knowledge.

This!
I always thought that broccoli build system is simple and powerful but it's nearly impossible to find any decent advanced documentation on how it works and how extend it. So any change towards better extension and ease of understanding is well accepted.
I'll read more carefully the RFC and try to provide useful comments XD

@dherman
Copy link

dherman commented Sep 14, 2017

Something I couldn't quite figure out from reading the RFC: does this API provide a mechanism for hooking into the built-in ember-cli build pipeline, or only to replace it entirely? In particular what I'm interested in is being able to hook in to the part of the default pipeline where modules have been parsed, and access their ASTs. If plugins have to replace the pipeline entirely then if they need semantic information from ASTs they'd have to parse the source files themselves, resulting in everything in the whole directory tree being parsed twice.

@twokul
Copy link
Contributor Author

twokul commented Sep 14, 2017

@dherman thanks for reading!

does this API provide a mechanism for hooking into the built-in ember-cli build pipeline, or only to replace it entirely?

As it stands right now, it merely introduces a way for developers to have granular control over bundling. At the moment, developers have no say in what goes where, how many bundles get produced, what to drop and what to include.

By introducing strategies, we allow for this flexibility and don't break the current behaviour of the build pipeline.

In particular what I'm interested in is being able to hook in to the part of the default pipeline where modules have been parsed, and access their ASTs. If plugins have to replace the pipeline entirely then if they need semantic information from ASTs they'd have to parse the source files themselves, resulting in everything in the whole directory tree being parsed twice.

This is something @rwjblue and I discussed as well but I decided to not include it in this RFC as it would create more questions than provide answers.

In order for us to incorporate fully functional treeshaking into the build pipeline, we absolutely need ASTs and don't want to parse files twice. The tricky part here is to make sure we expose the right public API w/o shooting ourselves in the leg. This API requires not only more experimentation but changes to the way the build pipeline works today.

Having said all of that, we want to make this API a reality and could definitely benefit from real life use cases like the one you mentioned.

@twokul
Copy link
Contributor Author

twokul commented Sep 14, 2017

@dherman to expand on my previous answer a little. Use case that you mentioned was one of the main drivers behind passing an instance of Assembler to toTree function. We could iterate on the specific APIs within Assembler that will allow to access ASTs.

@luxzeitlos
Copy link

Does the assembler call the strategies in parallel or in seriel? If it's run in parallel, how to modify the final output, and if it's run in seriel, wouldn't need the strategy to provide the same directory structure as output as specified as input for strategies? And how is that different from the abstraction we already have on broccoli layer, which maps input trees to output trees? Isn't that exactly what a structure does?

@twokul
Copy link
Contributor Author

twokul commented Sep 15, 2017

@luxferresum

Does the assembler call the strategies in parallel or in seriel?

It doesn't run in parallel. It might be rather challenging to coordinate that b/c Broccoli itself does not have such primitives and I don't think there are any plans to support that.

wouldn't need the strategy to provide the same directory structure as output as specified as input for strategies?

Not really, strategies get access to the same input tree, they are not chained off of each other.

And how is that different from the abstraction we already have on broccoli layer, which maps input trees to output trees?

I'm not too sure what abstraction you're referring to here, I'm going to assume it's the way we use Broccoli within Ember CLI. If so, the idea behind strategies is to make it easier to the way input trees are mapped to output trees and hence have control over the final bundles.

Let me know if I misunderstood your questions!

@luxzeitlos
Copy link

Thanks for your answer!

strategies get access to the same input tree, they are not chained off of each other.

Thats what I mean with running "in parallel". Sorry for being unclear, I basically wanted to ask if they are chained off on each other or run independent, and the final output of all strategies is merged in the end. So thanks for clarifying this.

However this keeps my subsequent question unanswered:

how to modify the final output

If the strategies are applied independent of each other, there is no way to modify the output of a "default" strategy and doing any kind of post-processing. Is this correct? Same is true for preprocessing.

Also this is very confusing for me because it does not match the idea of a car assembly line:

Assembler is similar to a car assembly line - starting with a metal frame, making changes to it along the way, adding parts and at the end, there's an assembled car.

This suggest that the car is build step by step, starting from the metal frame, making changes on the way, step by step, each step using the output of the previous step and adding something until I have a car. So This suggests the same for the ember Assembler, that it would run the first strategy, and then all others, each depending on the previous one, building an an assembly line from the development directory tree to the final build output.

So either the comparison of the Assembler and a assembly line is very wrong, or I'm missing something here?

Let me try to clarify. Either you run the strategies in subsequent series:

->strategyA->strategyB->strategyC->

Or independent:

    __> strategyA ___
   /                  \
__/___> strategyB _____\__>
  \                    /
   \__> strategyC ____/

If you run them independent, how to do post processing and how is this an assembly line?


my reference to the abstraction layer is the Broccoli Plugin Layer itself. If I understand a strategy basically maps one inputTree to one outputTree! And thats the same that a broccoli plugin does (simplified):

treeWithCss = compileSass(treeWithSass)

Then how is strategy different from a broccoli plugin? I'm expecting most strategies to be just wrappers around a broccoli plugin, or maybe a series of broccoli plugins.

@twokul
Copy link
Contributor Author

twokul commented Sep 18, 2017

@luxferresum sorry for the delayed response and thanks for explaining. I can see how it could be confusing now 😕

Also this is very confusing for me because it does not match the idea of a car assembly line:

They are indeed different. If we go along with a car-related analogy here, it's more of a service shop where the whole car gets "decorated" (repaint, rims/wheels change, etc.) 🚗 Let me change that part and thanks for pointing it out!

Then how is strategy different from a broccoli plugin? I'm expecting most strategies to be just wrappers around a broccoli plugin, or maybe a series of broccoli plugins.

The underlying primitive is indeed a broccoli plugin (it has nothing to do with strategies, that's how broccoli works). And yes, strategies will likely end up being wrappers around broccoli plugins. Going down the "everything is a broccoli plugin" path would lead to even further coupling of Ember CLI and Broccoli which actually doesn't have to be so. Strategy is more of a build pipeline primitive that can hide the build tool completely; "register a transform and it will happen" kind of thing. That way we could theoretically abstract "common" parts of CLI and make it less ember specific but keep interfaces between components in place.

Let me know if my answers made sense and let me rephrase parts of the RFC to include your feedback! 💪

@luxzeitlos
Copy link

Thats for your answer! Well, the one thing I still don't understand is how I would do something like a postprocessing with an strategy? Or is this out of scope?

@twokul
Copy link
Contributor Author

twokul commented Sep 18, 2017

Well, the one thing I still don't understand is how I would do something like a postprocessing with an strategy?

strategies aren't meant to be used for postprocess transformations; they are just solutions to different problems. The mental model here would be: postprocessTree is called after strategies have been processed.

Or is this out of scope?

at this point it is but I think it's good to disambiguate in the RFC

Alex Navasardyan added 2 commits September 18, 2017 11:47
Dave Herman and I spoke offline about some of the definitions in `Terminology`
section. Rephrasing to make it clear abit.
new FastbootStrategy(),
createDefaultApplicationStrategy(app),
...createDefaultVendorStrategy(app)

Copy link
Member

Choose a reason for hiding this comment

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

node 4 code here too

app.strategies = [
new Noop(),
createDefaultApplicationStrategy(app)
].concat(createDefaultVendorStrategy(app));
Copy link
Member

Choose a reason for hiding this comment

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

Why does createDefaultVendorStrategy return an array of strategies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's mostly have to do with the fact that vendor can produce several files, not just one.

@ro0gr ro0gr mentioned this pull request Nov 15, 2017
16 tasks
@twokul twokul changed the title Strategies Packaging Nov 17, 2017

# Terminology

+ **Packaging** - A process during of designing, evaluating, and producing final build assets.
Copy link
Member

Choose a reason for hiding this comment

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

"The process of" might be what you mean?

in Lisp. The gist of the idea is "how about we start using _only_ the code that
we actually need?"

Secondly, how is it different from [Dead Code
Copy link
Member

Choose a reason for hiding this comment

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

You can similarly explain this as dead code elimination is an imperfect outside-in approach, and tree-shaking is a (near) perfect inside-out approach.


# Unresolved questions

+ Will it increase build time?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we will be doing any extra work.

# Unresolved questions

+ Will it increase build time?
+ Should we introduce the same API on add-on level?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, since the end goal is for the app to compile addon code during it's tree-shaking step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, yes. I was toying around with the idea of maybe exposing some of the dep graph APIs on the add-on level. Having said that, it might be too far in the future


module.exports = function(defaults) {
const app = new EmberApp(defaults, {
package(inputTree) {
Copy link
Member

Choose a reason for hiding this comment

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

I still think we should still explore breaking this into two: transform and package. Then we could eliminate the packageFor and defaultPackager API. If someone needs to tree-shake and not mess with the packaging, they would use transform. If they need to modify the packaging, they weren't going to use the defaults anyways so they could just use package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to introduce as few new APIs as possible. I very much agree with idea of separating side effects (for example treeshaking) from packaging and I’m not opposed to “transform”. However it sounds very generic. Also, the long term goal here is to make treeshaking “hidden” from the developers perspective exposing only either some form of configuration or dependency graph itself.

If understood you correctly, the idea is to swap “packagerFor” with “transform”? Do you see any downsides of introducing “packagerFor” API?

const defaultPackager = packagerFor(app);
// customise `inputTree`

return defaultPackager(inputTree);
Copy link
Member

Choose a reason for hiding this comment

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

Assuming the package function receives a precompiled tree, if someone wanted to do custom bundling for their app, would they be opting out of our compilation system? Would they be responsible for doing their own compilation, even though they didn't want to affect the compilation step?

ES6 modules are [starting](https://caniuse.com/#feat=es6-module) to land in browsers.
This means that you can use `<script type="module" src="/my/app.js"></script>`.

This [article](https://philipwalton.com/articles/deploying-es2015-code-in-production-today/) [explains](https://philipwalton.com/articles/deploying-es2015-code-in-production-today/#is-this-really-worth-the-extra-effort) the benefits of using ES6 modules over ES2015 (smaller total file sizes, faster to parse and evaluate).
Copy link
Member

Choose a reason for hiding this comment

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

ES6 modules over ES2015

🤔

+ `addon-tree-output` is a folder that contains dependencies from Ember add-ons
+ `the-app-name-folder` is a folder that contains Ember application code
+ `node_modules` is a folder that contains node dependencies
+ `vendor` is a folder that contains other dependencies.
Copy link

Choose a reason for hiding this comment

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

Have we thought about including the test tree here?

We may want to produce separate bundles for lazy loading tests between an engine and the host

+ Rename `packagerFor` to `defaultPackager`
+ Introduce a new `ember-cli-default-packager` package
+ Add `tests` folder to the output structure
```javascript
// ember-cli-build.js
const EmberApp = require('ember-cli/lib/broccoli/ember-app');
const defaultPackager = require('ember-cli-default-packager');
Copy link

Choose a reason for hiding this comment

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

Does it mean ember-cli-default-packager has to be listed in the project dependencies?

bundler:js:input/
├── addon-tree-output/
├── the-app-name-folder/
├── node_modules/
Copy link

Choose a reason for hiding this comment

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

you need to include tests here in the diagram

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the current directory structure, not the new one

@rwjblue
Copy link
Member

rwjblue commented Mar 29, 2018

Moving to FCP...

@Turbo87 Turbo87 merged commit 5672016 into ember-cli:master Apr 5, 2018
@twokul twokul deleted the tree-shake-rfc branch April 5, 2018 20:36
@devinrhode2
Copy link

Looks like the Rendered link at the top is broken :(

@btecu
Copy link

btecu commented Jun 7, 2018

You can just click Files changed and then View:
https://github.com/twokul/rfcs-1/blob/d26cb1ff75f78c83fcbabe7de57f365a970f120f/active/0051-packaging.md

@buschtoens
Copy link

buschtoens commented Nov 23, 2018

I've been starting to experiment with the API have one small complaint regarding ergonomics for the end user: It appears to not be possible for an addon to hook into the package hook. Users have to explicitly require and pass the packager to the EmberApp.

EDIT: I lied. 😂 You can access / override parent.options.package in the included hook just fine.

module.exports = {
  name: require('./package').name,

  included(parent) {
    // Assuming this addon is directly included by the host app.
    // Otherwise you need to traverse `parent` upwards.
    parent.options.package = function(inputTree) {
      // Do what you want with `inputTree` here or just forward to the default packager implementation.
      // https://github.com/ember-cli/ember-cli/blob/v3.6.0-beta.1/lib/broccoli/ember-app.js#L1765-L1767
      return this._legacyPackage(inputTree);
    };

    this._super.included.apply(this, arguments); 
  }
}

kategengler pushed a commit to kategengler/rfcs that referenced this pull request Jan 19, 2019
xg-wang referenced this pull request in emberjs/rfcs Feb 10, 2019
- Misc frontmatter cleanup on those RFCs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet