Skip to content
This repository has been archived by the owner. It is now read-only.

Add building targets RFC. #75

Merged
merged 2 commits into from Jan 5, 2017

Conversation

Projects
None yet
7 participants
@kratiahuja

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2016

Please refer to the comments from this issue on some of the feedback: ember-cli/ember-cli#6350


Even though this proposal is very transparent to the existing ember-cli semantics, it does have two main drawbacks:
1. This generates additive assets which can be only used with the browser builds only. In order for the additive assets to override the existing behavior of the browser targets, we assume the loader overrides the modules with the same name based on the ordering. If the behavior of the loader changes in future, this will break the concept of creating additive asset.
2. If you are loading the additive assets in script tags (not valid for fastboot usecase) and for some reason the network request for fetching the additive asset fails, your app will be in an inconsistent state for that target environment. In order to play safe, the addon could merge the generated app.js and app-<targetname>.js files. This is more of a design design decision of the target environment that wants to use the additional generated assets.

This comment has been minimized.

Copy link
@masonwan

masonwan Nov 2, 2016

Typo

"... a design design decision ..."


_Note_: This is optional item for this RFC.

# How We Teach This

This comment has been minimized.

Copy link
@masonwan

masonwan Nov 2, 2016

Looks like a typo of "Reach".

This comment has been minimized.

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 2, 2016

Author Contributor

It is supposed to be "Teach". It is present in the RFC template. I didn't add that section title.


# How We Teach This

This is an optional feature that only certain addon/apps want to have. This is a backward compatible change to the existing ember eco system. I would like to propose introduce the concept of `targets` since it makes it clear there are additional target that an addon/app may introduce and want to use. Since this allows to create a parallel additional build it keeps the API and understanding to the user same as before. In order to teach users how to use targets, we need to update the API docs with a section for this and the best practices of when to use this.

This comment has been minimized.

Copy link
@acorncom

acorncom Nov 2, 2016

Member

Thinking aloud here, could we add a section to the generated add ons / apps with the fastboot "target" environment commented out (along with a link to more docs on this feature)?

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

I assume you mean the generated README that gets created when an app/addon is created?

I was also thinking to add this in ember-welcome-page addon as an optional annoucement.

This comment has been minimized.

Copy link
@acorncom

acorncom Nov 3, 2016

Member

Not the README, but potentially the ember-cli-build file. The hard part is it'd be nice for folks to know about it but it's an "advanced user feature". As a result, we have to be careful to not expose too many of the "knobs" that you adjust to begin with.

In that regard, I'm definitely NOT a fan of adding it to the welcome page. Folks who see the welcome page are generally brand new to Emberand don't need to be worrying about Fastboot builds 😀

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

aah ok, yes we can. Yes I didn't realize that ember-cli-welcome page is for new users.

@kellyselden

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

Have you considered how this affects #35. Do you see it replacing or supplementing it?

```
The above config means that this ember-addon wants to define an additional target named "fastboot" which depends on the "browser" target. The depends on means that whenever this target is built, it needs to make sure the dependent target is also built so that the assets can be served correctly in the target environment.

If an app wants to define a new target, it will specify the new target in `ember-cli-build.js` when it creates the ember app:

This comment has been minimized.

Copy link
@kellyselden

kellyselden Nov 3, 2016

Member

I'd like to try and figure out how to unify both app and addon target definitions. Either both in the package.json or both in config files. I think having two different places will cause confusion.

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

I didn't find a clean way of unifying the two definitions. App doesn't have ember specific configuration in its package.json AFAIK. This was the cleanest way that I could think of. Note we need ember-cli eco-system to know the extra assets before the build starts. If you have any suggestions, please let me know.

```

Note:
1. The command `ember serve` will remain as it is today since it serves the assets based on what is requested. If the targeted assets need to be served, the app/addon should update the index.html or their environment in which this is loaded to be served. The targeted app and addon directories should be watched so that a change in them triggers a rebuild.

This comment has been minimized.

Copy link
@kellyselden

kellyselden Nov 3, 2016

Member

I don't see how this cleanly works. The index.html js and css loading are static to two statements each. It doesn't have a way to dynamically load the additional target files. If your generate target command added lines to this file, then it is now permanently configured for the target only, and running in browser mode would try to request the target files and 404.

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

How the target files should be loaded is on the app itself. The reason being it is not necessary an app will want to load the target files in the browser mode. For example, fastboot doesn't require the target files to be loaded in the browser but in its sandbox Node VM.

If an app wants to load the targeted files in the browser, they can always merge the app.js and app-target.js into a new app.js and load that. If they don't wish to merge, they can always update app/index.html with additional script tags.

That is the reason I don't like the idea of updating ember serve command since not all apps may want to use the targeted files in browser mode.

This comment has been minimized.

Copy link
@kellyselden

kellyselden Nov 3, 2016

Member

Ah thanks for clarifying. I get the picture now!

## Public hooks
`ember-cli` as part of the build pipeline exposes public API for addons to change the different trees and processing per their needs. Since the targeted builds are doing an additional builds as a compliment to the existing browser build, we need to expose similar APIs for the target builds as well. Following are the public apis we need to expose:

- `preProcessTree(type, tree)`: The existing type for browser build is represented by `javascript`, `styles` etc. We need to expose additional types when targetted builds are run. Therefore, for a target build the type will be invoked as `javascript-<targetname>`, `styles-<targetname>` etc.

This comment has been minimized.

Copy link
@kellyselden

kellyselden Nov 3, 2016

Member

We may want to consider moving platform to a third parameter, or moving to an options hash so order is never an issue again. Then we can test first param if it is string or object, and be backwards compatible.

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

I am not sure I follow this. I am not changing the existing API but adding more types.

## config environments
By default ember exposes an environment file which contains configuration in different environments (development, test and production). If a target wants to override a configuration in its environment, it should be allowed to do so. Therefore, this RFC proposes an additional directory structure
```
config/

This comment has been minimized.

Copy link
@kellyselden

kellyselden Nov 3, 2016

Member

Could this also be implemented as, use the current environment.js, just supply a second param of target?

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

But what if someone wants to override the entire environment.js in the target environment? (Note: Fastboot doesn't have this usecase, hence this is an optional item in the RFC). I am fine with dropping this section also.

@kellyselden

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

We also might want to debate whether target or platform is the best term. Learning team involvement would be great. cc @locks

@kellyselden

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

Also I wouldn't mind getting @runspired involved, since this is a reintroduction of his platforms RFC. Do you think they should be merged? Do you think one should be superseded?

@stefanpenner

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

@rwjblue / @dgeb we need your 👀 on this, to understand impact on modules RFC

@stefanpenner

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

source issue and idea, I strongly recommend reviewers read this to get context: ember-fastboot/ember-cli-fastboot#264 (comment)

Although this is exploring something useful, it seems to be expanding well beyond the original intent. If we aim to ship the original thing (enabling fastboot) we will need to tune this down to address minimum requirements to ship: ember-fastboot/ember-cli-fastboot#264 (comment) + similar pattern for fastboot folder in addons.

"configPath": "tests/dummy/config",
"additionalTargets": {
"fastboot": {
"dependsOn": ["browser"]

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Nov 3, 2016

Contributor

This seems interesting, but I suspect a first pass only allows: 1 target level depth, extending the base app. Yes this may mean in the future we want more, but with that growing pain we will also have learned the required lessons to correctly solve the problem.

The goal of this effort (right now) is not a general purpose solution, it is to allow for ancillary files being added for fastboot.

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 3, 2016

Author Contributor

+1 to 1 target level depth. I don't want to complicate the RFC without knowing the all generic usecases. When I wrote the RFC what I had in mind was to keep it simple such that:

  1. It primarily solves fastboot use case
  2. Is some what generic enough that it can be extended later

I know #2 was not intended in the original idea but I think this will let fastboot also iterate its builds easily.

This comment has been minimized.

Copy link
@kratiahuja

kratiahuja Nov 11, 2016

Author Contributor

@stefanpenner I believed I addressed your feedback in a separate RFC. I would still like to keep this RFC open for feedback since I believe this RFC should be the long term support.

@kratiahuja

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

@kellyselden I wouldn't want to supersede or merge @runspired RFC . The reason being this RFC is primarily meant to only solve fastboot usecase and should be generic enough to extend to other platforms/targets.

I read #35 but I don't understand how electron and cordova environments work or are designed so I wouldn't want to add generic APIs without a clear understanding. I agree we should have @runspired give feedback on this RFC since he has most knowledge about other platforms.

However, just to be clear the goal of this RFC is to just solve fastboot usecases. So I wouldn't prefer to make this very generic until we know what we want eventually. This RFC is the minimal public API work from ember-cli to support fastboot usecase only and ability to extend and iterate later.

@nathanhammond nathanhammond merged commit c42f7e2 into ember-cli:master Jan 5, 2017

@nathanhammond

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

Hey everybody, this RFC was accidentally closed, but it is not merged. It has been reopened at #93. Conversation should continue there, I'm locking this thread.

@ember-cli ember-cli locked and limited conversation to collaborators Jan 6, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.