-
Notifications
You must be signed in to change notification settings - Fork 160
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
Add more declarative way of adding/removing assets for fastboot #413
Comments
Related to #387 |
Often easy and simple aren't the same things so we may require additional knowledge here, likely a two pronged approach is appropriate:
|
I'll take a stab at this over the weekend. I have an idea in my mind but want to spike out and think aloud. |
@scottmessinger Thanks for the write up and your thoughts! Really appreciate it since it helps us improve things before releasing. Reading the original issue that you posted, I would like to call it out very clearly that there is NO two different builds/environments any longer. Assets aren't any longer forked for browser and fastboot that you can chose if a lib should be included or not included in fastboot/browser. That's the reason we are proposing to wrap libraries to be only run in browser with the FastBoot check. Whichever assets are loaded in browser are loaded in fastboot too. You can additionally load overrides or additional library in fastboot. That's how the new build works. I think the confusion is more around the assumption that fastboot is still running two builds when it is not. Also to that point, When refactoring the build to make it performant and shippable as a 1.0 candidate, we tried hard to keep it backwards compatible, however this wasn't easy as it sounds. To answer your original concern of the first two migration path, here is what I propose: Case 1: Running a library only in browser environmentI created a utility in github called fastboot-transform which wraps the stew magic away. This should address your concern about "require no new broccoli plugins like broccoli-stew". In order to import a library that is only meant to run in browser you could do currently use treeForVendor(tree) {
// as addon author you already were doing this
var browserLib = new Funnel(..);
return new MergeTrees([tree, fastboot-transform(browserLib)]);
}
included(app) {
app.import(path); ImprovementA substantial improvement to the above scheme is proposed via the treeForVendor(tree) {
//everything remains same as you had pre rc builds. you don't need to even use fastboot-transform here
}
included(app) {
app.import(path, {
using: [{
transformation: fastboot-transform
}]);
} This would work well for bower libraries and node_modules that are imported as well. Case 2: Running a library only in fastbootI disagree here that addon authors that wish to make their addons fastboot complaint should not have knowledge about fastboot. As addon authors if you want to make your addon compatible to be running in fastboot, you need to be aware of the public API fastboot is exposing.
The simplest example of before and after the refactor is: included(app) {
if (process.env.EMBER_CLI_FASTBOOT) {
app.import(fastbootLibPath);
}
} AFTER: included(app) {
app.import(fastbootLibPath, { outputFile: 'assets/fastboot-lib.js' }
}
updateFastBootManifest(manifest) {
// load my fastboot lib with other vendor files
manifest.vendorFiles.push('assets/fastboot-lib.js');
return manifest;
}
If you think var fastbootUtility = require('ember-cli-fastboot/fastboot-build-utility');
included(app) {
fastboot.import(path);
} Happy to review if someone wants to PR it. Migration guideWhile I totally agree the migration guide is not in its best state but @tsubomii is working on improving it. We can probably post some real world examples of addons that have already done the migration in order to help addon authors to migrate their addons. This would greatly reduce the friction for addon authors and would instill more confidence in the migration. Do you think that would help? Any other ideas or help on improving the guide/docs or migration are greatly appreciated! |
Thanks, @kratiahuja! A few response below:
Agreed about the builds and the difference in the use of "environments" might just be semantics. With fastboot, I think of my app as having the same build (same concatenated javascript files) running in two environments: node & the browser.
Thanks again for all your hard work on this. Case 1 improvement
I think this looks great! My one suggestion was the name: "fastboot transform" makes me think it's a generic transform with some generic positive outcome and not "put a guard around this script so it doesn't run in node." Again, the name might be perfect as it's intended to be a generic transform. But, if the goal is specific, might I suggest a more pointed name. Perhaps, Case 2: Running only in fastboot
I agree with you and I think I misspoke early. I think your point is exactly right, "you need to be aware of the public API." I went back and looked at the original gist and I think it was the combination of three callbacks In answer to your questions, "updateFastBootManifest is a bit wrongly named or verbose", I think its fine. The two steps of importing the file and then adding to the fastboot manifest seem reasonable. Migration guideJust to clarify, the gist was always great -- it correctly exposed the complexity of the migration and help facilitate the conversation about improving it! I think the changes above address my concerns and I'm so grateful for the time & work you've put into this, @kratiahuja. |
The new API to make easy for apps to import fastboot incompatible libraries is merged. For addon, I will provide an API in Going to close this. |
Problem:
The current guide to migrating fastboot is very helpful, but requires understanding far more about broccoli and the fastboot manifest than in the past. Because of the increased knowledge, updating an addon to be compatible with fastboot 1.0 is challenging. Ryan had an experience similar to mine:
Specifically, the migration paths that have proved the most challenging are importing in fastboot build and importing for a browser build. Look at the first two uses cases in the upgrading gist for more.
This problem also affects developers seeking to make their apps compatible for fastboot by including or excluding libs for fastboot.
Goals:
While having low level primitives is very useful for edge cases, creating a more declarative api for including an asset for browser or fastboot would make the migration easier and reduce the amount of knowledge addons authors have to learn. By lowering the barriers, new addons will be more likely to be fastboot compat out of the gate and existing addons will be more quickly migrated.
Ideally, a solution would:
updateFastBootManifest
)A possible solution:
I'm not knowledgable enough of ember-cli to know the best path forward, but as a novice, I'd love to see something like:
The text was updated successfully, but these errors were encountered: