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.

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

Remove hardcoded index.html value #2523

Merged
merged 2 commits into from
Nov 19, 2014
Merged

Remove hardcoded index.html value #2523

merged 2 commits into from
Nov 19, 2014

Conversation

teoljungberg
Copy link

This allows the generated html file to be renamed however is needed.

This is exceptionally helpful for configuring how the root HTML file
should be cached, since the index.html is cached harshly by most web
servers

To use it, configure outputPaths.app.html in your Brocfile.js file:

var EmberApp = require('ember-cli/lib/broccoli/ember-app');

var app = new EmberApp({
  outputPaths: {
    app: {
      html: 'custom-name.html'
    },
  }
});

module.exports = app.toTree();

@stefanpenner
Copy link
Contributor

tests please

@rwjblue
Copy link
Member

rwjblue commented Nov 18, 2014

I would prefer to use outputPaths (added in #1904).

var files = [
'index.html'
HTMLRootFile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexPath or indexName ?

@stefanpenner
Copy link
Contributor

if we make this change, it will need to also submit a PR to the website.

@rwjblue
Copy link
Member

rwjblue commented Nov 18, 2014

Basically something like:

/* global require, module */
var EmberApp = require('ember-cli/lib/broccoli/ember-app');

var app = new EmberApp({
  outputPaths: {
    app: {
      html: 'some-other-thing.html'
    }
  }
});

module.exports = app.toTree();

@stefanpenner
Copy link
Contributor

@rwjblue +1

@teoljungberg
Copy link
Author

@rwjblue 👍

@wayne-o
Copy link

wayne-o commented Nov 18, 2014

This would also really help for apps that are hosted in something like ServiceStack where the index.html needs to be serverside rendered to pre-populate a preloadStore

@teoljungberg
Copy link
Author

I'll modify the PR to use the outputPath instead /cc @stefanpenner @rwjblue

@stefanpenner
Copy link
Contributor

tests please, and docs in a separate PR to the website. I would like to get it in for tonight's potential release.

@stefanpenner stefanpenner added this to the 0.1.3 milestone Nov 18, 2014
@teoljungberg
Copy link
Author

PR updated with tests
The PR for the documentation can be found here - #2528

@stefanpenner
Copy link
Contributor

👍 lGTm

@@ -269,7 +269,7 @@ describe('Acceptance: brocfile-smoke-test', function() {
});
});

it('specifying custom output paths works properly', function() {
it.only('specifying custom output paths works properly', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it.only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch.

@teoljungberg
Copy link
Author

@rwjblue I can't really get the idea to work that If we want to keep the index.html file intact and build to custom-name.html.

If I keep index.html intact, and want to build to the outputPaths.app.html file

diff --git a/lib/broccoli/ember-app.js b/lib/broccoli/ember-app.js
index 53107f5..ade119c 100644
--- a/lib/broccoli/ember-app.js
+++ b/lib/broccoli/ember-app.js
@@ -280,7 +280,7 @@ EmberApp.prototype.populateLegacyFiles = function () {
 EmberApp.prototype.index = function() {
   var htmlName = this.options.outputPaths.app.html;
   var files = [
-    htmlName
+    'index.html'
   ];

   var index = pickFiles(this.trees.app, {
diff --git a/tests/acceptance/brocfile-smoke-test-slow.js b/tests/acceptance/brocfile-smoke-test-slow.js
index eda4fe0..9ee7e38 100644
--- a/tests/acceptance/brocfile-smoke-test-slow.js
+++ b/tests/acceptance/brocfile-smoke-test-slow.js
@@ -282,13 +282,6 @@ describe('Acceptance: brocfile-smoke-test', function() {
         var appCSS = fs.readFileSync(appCSSPath);
         return fs.writeFileSync(themeCSSPath, appCSS);
       })
-      .then(function () {
-        // copy index.html to my-app.html
-        var indexHTMLPath = path.join(__dirname, '..', '..', 'tmp', appName, 'app', 'index.html');
-        var myAppHTMLPath = path.join(__dirname, '..', '..', 'tmp', appName, 'app', 'my-app.html');
-        var appHTML = fs.readFileSync(indexHTMLPath);
-        return fs.writeFileSync(myAppHTMLPath, appHTML);
-      })
       .then(function() {
         return runCommand(path.join('.', 'node_modules', 'ember-cli', 'bin', 'ember'), 'build', {
           verbose: true

Then the tests fails fail. I agree that that's the behavior we want. But my knowledge about the ember-cli build process is really too limited. I could really use some help and pointers to get this PR working and merged

@teoljungberg
Copy link
Author

@rwjblue I executed a crazy idea in mynewsdesk/ember-cli@21f8ebb that seems stable. Thoughts?

@@ -287,9 +289,21 @@ EmberApp.prototype.index = function() {
destDir: '/'
});

index = new Funnel(index, {
srcDir: '/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default for broccoli-funnel, no need to specify.

@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2014

@teoljungberg - Crazy like a fox ;) That is exactly what I was typing up as a suggestion!

@teoljungberg
Copy link
Author

@rwjblue pushed and fixed. Deleting code is always satisfying

var files = [
'index.html'
];

var index = pickFiles(this.trees.app, {
index = new Funnel(this.tress.app, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need the var here I think

@teoljungberg
Copy link
Author

@rwjblue I rebased against master and pushed fixes to your comment

@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2014

Awesome! Can you squash the commits? Then I'll hit the big green button once Travis passes...

@teoljungberg
Copy link
Author

@rwjblue sure thing - consider it done!

Nicolas Medda + Teo Ljungberg added 2 commits November 19, 2014 14:20
This allows the generated html file to be renamed however is needed.

This is exceptionally helpful for configuring how the root HTML file
should be cached, since the `index.html` is cached harshly by most web
servers

To use it, configure `outputPaths.app.html` in your `Brocfile.js` file:

    var EmberApp = require('ember-cli/lib/broccoli/ember-app');

    var app = new EmberApp({
      outputPaths: {
        app: {
          html: 'custom-name.html'
        },
      }
    });

    module.exports = app.toTree();
@teoljungberg
Copy link
Author

I force pushed over a few typos and squashed the commits into 2, 1 for the actual change and 1 for the addition to the changelog @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2014

Awesome, thank you!

rwjblue added a commit that referenced this pull request Nov 19, 2014
@rwjblue rwjblue merged commit 4203698 into ember-cli:master Nov 19, 2014
@knownasilya
Copy link
Contributor

👍 Thank you very much @teoljungberg, been looking for something like this. Tried to implement myself, but not familiar enough with Broccoli.

@gpoitch
Copy link

gpoitch commented Nov 30, 2014

Thanks for this. Wouldn't we want the express dev server to use this configuration to serve its root file? Or is that not always desirable?

@rwjblue
Copy link
Member

rwjblue commented Nov 30, 2014

@gdub22 - Yes, we absolutely would like the express server to use this for its history support. Unfortunately, configuration made in the Brocfile.js is not available in the express server. This is yet another issue to be solved by the upcoming configuration refactoring.

@jayphelps
Copy link
Member

@teoljungberg Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants