Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Runs after broccoli-asset-rev #2

Open
ef4 opened this issue Nov 2, 2017 · 1 comment
Open

Runs after broccoli-asset-rev #2

ef4 opened this issue Nov 2, 2017 · 1 comment

Comments

@ef4
Copy link
Owner

ef4 commented Nov 2, 2017

Assets URLs in the prebuilt HTML don't get rewritten to point at fingerprinted assets, because prember is running after broccoli-asset-rev.

This would be simple to fix by setting before: [ 'broccoli-asset-rev'] in our package.json, except that we also need to run after ember-cli-fastboot, and ember-cli-fastboot insists on running before broccoli-asset-rev.

$ ember build --environment=production
cycle detected: prember <- ember-cli-fastboot <- broccoli-asset-rev <- prember
ef4 added a commit to ef4/broccoli-asset-rev that referenced this issue Nov 3, 2017
@ef4
Maintain correctness when rewriting fastboot-capable apps
838f1b7 
Apps that are built with fastboot support contain a fastboot.manifest in their package.json. The manifest includes the names of JS and HTML files. These files may be renamed by broccoli-asset-rev.

Up until now, ember-cli-fastboot has been doing hacks to handle adjusting those references itself. But this requires it to have too much knowledge of broccoli-asset-rev, and it forces ember-cli-fastboot to run *after* broccoli-asset-rev, which is problematic for use-cases like ef4/prember#2.

This PR reverses that approach by considering fastboot.manifest a standard part of the built ember app that broccoli-asset-rev should know how to update for consistency when renaming files.

This PR includes a flag on the addon itself that we can use to migrate ember-cli-fastboot away from its hacky behavior.
ef4 added a commit to ef4/ember-cli-fastboot that referenced this issue Nov 3, 2017
@ef4
Disentangle from broccoli-asset-rev
89d589a 
ember-cli-fastboot has a lot of intimate knowledge of broccoli-asset-rev, and it's forced to always run after broccoli-asset-rev.

This increases complexity and reduces flexibility, resulting in problems like ef4/prember#2, in which we want to use the output of ember-cli-fastboot to render some content HTML which will then need to run through broccoli-asset-rev.

Rewriting assets is broccoli-asset-rev's responsibility, so we can just teach it how to properly rewrite a fastboot-capable ember app. I've done that work [here](https://github.com/ef4/broccoli-asset-rev#838f1b7186fde566d3e8010196c1f974423a4946).

This PR is the corresponding change in ember-cli-fastboot, which mostly just rips out unneeded code.

This is not an upgrade that can be done gracefully, however, since addons cannot dynamically change their `before` and `after` declarations, and my entire motiviation in doing this is to get rid of `after: ['broccoli-asset-rev']`. So this PR creates a hard requirement that the user has a correspondingly new broccoli-asset-rev. I added a clear error message so that it should be only a small stumbling block.

This PR will not pass the test suite, because ember-cli-addon-tests is apparently incapable of being told what dependencies to use. Perhaps it would be best to simply wait to see if we can get the newer broccoli-asset-rev released and upgraded in the default app blueprint before merging this.
@ef4
Copy link
Owner Author

ef4 commented Nov 3, 2017 •
edited
Loading

This is addressed in ddbeae1.

Which I have so far released only as an alpha, because it depends on PRs to broccoli-asset-rev and ember-cli-fastboot that are yet to be filed/merged/released.

If you want to try it out, you can use these dependencies:

{
    "broccoli-asset-rev": "https://github.com/ef4/broccoli-asset-rev#838f1b7186fde566d3e8010196c1f974423a4946",
    "ember-cli-fastboot": "https://github.com/ef4/ember-cli-fastboot#89d589a2e4edb4e0eb432ed7cc8680237019aa12",
    "prember": "0.3.0-alpha.0"
}

ef4 added a commit to ef4/broccoli-asset-rev that referenced this issue Nov 3, 2017
@ef4
Maintain correctness when rewriting fastboot-capable apps
4e9aea2 
Apps that are built with fastboot support contain a fastboot.manifest in their package.json. The manifest includes the names of JS and HTML files. These files may be renamed by broccoli-asset-rev.

Up until now, ember-cli-fastboot has been doing hacks to handle adjusting those references itself. But this requires it to have too much knowledge of broccoli-asset-rev, and it forces ember-cli-fastboot to run *after* broccoli-asset-rev, which is problematic for use-cases like ef4/prember#2.

This PR reverses that approach by considering fastboot.manifest a standard part of the built ember app that broccoli-asset-rev should know how to update for consistency when renaming files.

This PR includes a flag on the addon itself that we can use to migrate ember-cli-fastboot away from its hacky behavior.
This was referenced Nov 3, 2017
Merged
Merged
ef4 added a commit to ef4/ember-cli-fastboot that referenced this issue Feb 12, 2018
@ef4
Disentangle from broccoli-asset-rev
5fae5bd 
ember-cli-fastboot has a lot of intimate knowledge of broccoli-asset-rev, and it's forced to always run after broccoli-asset-rev.

This increases complexity and reduces flexibility, resulting in problems like ef4/prember#2, in which we want to use the output of ember-cli-fastboot to render some content HTML which will then need to run through broccoli-asset-rev.

Rewriting assets is broccoli-asset-rev's responsibility, so we can just teach it how to properly rewrite a fastboot-capable ember app. I've done that work [here](https://github.com/ef4/broccoli-asset-rev#838f1b7186fde566d3e8010196c1f974423a4946).

This PR is the corresponding change in ember-cli-fastboot, which mostly just rips out unneeded code.

This is not an upgrade that can be done gracefully, however, since addons cannot dynamically change their `before` and `after` declarations, and my entire motiviation in doing this is to get rid of `after: ['broccoli-asset-rev']`. So this PR creates a hard requirement that the user has a correspondingly new broccoli-asset-rev. I added a clear error message so that it should be only a small stumbling block.

This PR will not pass the test suite, because ember-cli-addon-tests is apparently incapable of being told what dependencies to use. Perhaps it would be best to simply wait to see if we can get the newer broccoli-asset-rev released and upgraded in the default app blueprint before merging this.
ef4 added a commit to ef4/ember-cli-fastboot that referenced this issue Mar 29, 2018
@ef4
Disentangle from broccoli-asset-rev
158af9e 
ember-cli-fastboot has a lot of intimate knowledge of broccoli-asset-rev, and it's forced to always run after broccoli-asset-rev.

This increases complexity and reduces flexibility, resulting in problems like ef4/prember#2, in which we want to use the output of ember-cli-fastboot to render some content HTML which will then need to run through broccoli-asset-rev.

Rewriting assets is broccoli-asset-rev's responsibility, so we can just teach it how to properly rewrite a fastboot-capable ember app. I've done that work [here](https://github.com/ef4/broccoli-asset-rev#838f1b7186fde566d3e8010196c1f974423a4946).

This PR is the corresponding change in ember-cli-fastboot, which mostly just rips out unneeded code.

This is not an upgrade that can be done gracefully, however, since addons cannot dynamically change their `before` and `after` declarations, and my entire motiviation in doing this is to get rid of `after: ['broccoli-asset-rev']`. So this PR creates a hard requirement that the user has a correspondingly new broccoli-asset-rev. I added a clear error message so that it should be only a small stumbling block.

This PR will not pass the test suite, because ember-cli-addon-tests is apparently incapable of being told what dependencies to use. Perhaps it would be best to simply wait to see if we can get the newer broccoli-asset-rev released and upgraded in the default app blueprint before merging this.
ef4 added a commit that referenced this issue Mar 29, 2018
@ef4
run after broccoli-asset-rev
9063aff 
Closes #2.

This depends on broccoli-asset-rev >= 2.7.0 and ember-cli-fastboot PR 546
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@ef4