-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conditionally allow assets to be loaded without a manifest. Update README to document available options. #89
base: master
Are you sure you want to change the base?
Conversation
… option is supplied by the host app or addon
app/config/asset-manifest.js
Outdated
@@ -1,24 +1,30 @@ | |||
import require from 'require'; | |||
import environment from './environment'; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove added whitespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Will do.
* Type: `boolean` | ||
* If `true`, a <meta> tag which would eventually hold the manifest will not be inserted into the DOM. | ||
|
||
##### noManifestLookup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the flag actually not lookup the manifest? Or is it just going to ignore any errors arising from a lookup? What is the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If noManifestLookup
is set to true
, then there will be no manifest lookup. The default value will be undefined
, meaning the manifest will be looked up by default (backwards compatible)
@@ -41,6 +41,16 @@ the asset manifest. This defaults to `[ 'js', 'css' ]`. | |||
_Note: This class provides default `contentFor`, `postprocessTree`, and `postBuild` hooks so be sure that you call | |||
`_super` if you override one of those methods._ | |||
|
|||
### Options | |||
|
|||
##### noManifest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the default value?
This PR is meant to address a part of #87 . In short, we are generating a manifest from our external addon because some of the host apps that consume our addon do not use engines nor do they provide their own manifest through other means. In the event that a consuming host app does use ember-engines we run into an issue where a manifest with the engine name is not generated.
This PR does not address the underlying issue but does solve our particular problem which is that our addon should not need to supply a manifest in the first place. I.e we'd like to re-use the lazy-loading functionality that ember-asset-loader provides without having to deal with manifest generation.
I could use a hand from the community to write a test against this new option. My thinking is that we could write a unit test in
asset-manifest-test.js
which callsgetManifest
on theasset-loader
service and verifies that no manifest was pushed when thenoManifestLookup
option is present. For the life of me I cannot remember how to grab a reference to a service from within a unit test using this old qunit module syntax. I'm also coming up empty-handed in the docs. Boththis.owner
andgetOwner(this)
both returnundefined
. I'd really appreciate some help here.