Add building targets RFC. #75
Conversation
|
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. |
There was a problem hiding this comment.
Typo
"... a design design decision ..."
|
|
||
| _Note_: This is optional item for this RFC. | ||
|
|
||
| # How We Teach This |
There was a problem hiding this comment.
@masonwan "Teach" is correct. This is part of the RFC template: https://github.com/ember-cli/rfcs/blame/26787f0e5900c3fa83a975992b68226f9f891dfa/0000-template.md#L22
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😀
There was a problem hiding this comment.
aah ok, yes we can. Yes I didn't realize that ember-cli-welcome page is for new users.
|
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/ |
There was a problem hiding this comment.
Could this also be implemented as, use the current environment.js, just supply a second param of target?
There was a problem hiding this comment.
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.
|
We also might want to debate whether |
|
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? |
|
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"] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+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:
- It primarily solves fastboot use case
- 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.
There was a problem hiding this comment.
@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.
|
@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. |
|
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. |
rendered
cc: @stefanpenner @tomdale @nathanhammond @ryanone @masonwan @kristoferbaxter @arjansingh @rwjblue @melmerp