Make insertion happen as a broccoli plugin. #16
Conversation
nathanhammond
force-pushed
the
broccoli-inserter
branch
6 times, most recently
from
71b798b to
a6487c6
Compare
nathanhammond
changed the title
|
No longer a WIP. @stefanpenner if you can review the new Broccoli plugin I'd appreciate it. |
nathanhammond
force-pushed
the
broccoli-inserter
branch
from
a6487c6 to
1242957
Compare
| } | ||
|
|
||
| if (fs.existsSync(indexFilePath)) { | ||
| var index = metaReplacer(fs.readFileSync(indexFilePath, { encoding: 'utf8'}), manifest); |
There was a problem hiding this comment.
Nit: space after 'utf8' and can we break this into two lines? Probably:
var indexFile = fs.readFileSync(...);
var indexWithManifest = metaReplacer(indexFile, manifest);Similar for the testIndex below.
nathanhammond
force-pushed
the
broccoli-inserter
branch
4 times, most recently
from
7329bcc to
3bf72e0
Compare
|
Now supports arbitrary |
|
@nathanhammond can you rebase this? Should fix the SauceLabs issue. Quick review doesn't show any major issues, will review and likely merge tomorrow assuming Sauce passes. |
nathanhammond
force-pushed
the
broccoli-inserter
branch
2 times, most recently
from
f8fbe65 to
2092b48
Compare
|
All set. |
| console.warn('\n\nWarning: Unable to read asset-manifest.json from build with error: ' + error) | ||
| console.warn('Warning: Proceeding without generated manifest; you will need to manually provide a manifest to the Asset Loader Service to load bundles at runtime. If this was intentional you can turn this message off via the `noManifest` flag.\n\n'); | ||
| manifest = { bundles: {} }; | ||
| if (assetLoaderOptions && assetLoaderOptions.noManifest) { |
There was a problem hiding this comment.
We can probably change this to be before we generate the asset manifest (likely worthy of another PR since this is the current behavior).
There was a problem hiding this comment.
I call dibs on you doing it. 😜
|
Some small comments. Once the above are addressed this is good to land. |
nathanhammond
force-pushed
the
broccoli-inserter
branch
2 times, most recently
from
8d8e678 to
d7fbf04
Compare
nathanhammond
force-pushed
the
broccoli-inserter
branch
from
d7fbf04 to
965f812
Compare
|
|
||
| AssetManifestInserter.prototype.build = function() { | ||
| var sourceDir = this.inputPaths[0]; | ||
| var manifestFilePath = path.join(sourceDir, 'asset-manifest.json'); |
There was a problem hiding this comment.
it seems like there are only 3 inputs that matter, yet the current usage will assume a rebuild is required if any of the inputs changes, also it will always taint the output even if the output has not changed.
I suspect we don't want either of these behaviors.
There was a problem hiding this comment.
@stefanpenner can you work with @nathanhammond to optimize this?
Edit: or provide some guidance on how to rectify and I'll take a stab eventually
There was a problem hiding this comment.
I thought I addressed that with this? https://github.com/trentmwillis/ember-asset-loader/pull/16#discussion-diff-77695562R23
meta-handler.AssetManifestInserterBroccoli plugin.