Find a better way to manage inline vs remote CSS

gauntface commented 8 years ago

Possible Options:

1.) Don't - just inline CSS 2.) Enforce some kind of naming convention

Default file is index_inline.scss and index_remote.scss if they exists
for a url, use the name after the origin. For example: localhost:9000/home would look for home_inline.scss and home_remote.scss 3.) Make some better helper methods to wrap request and response parameters (these would take in the response, a css name and view. Ensure safe defaults.

@addyosmani - thoughts? Other options?

addyosmani commented 8 years ago

Imo, we're going to need some convention for managing inline vs. remote. Whilst simplifying this down to just inline would initially make things simpler I think folks will eventually want some helpers for the remote side of things. It'd be good to have them covered with some rails.

I initially favour 2) more than 3), though if you have more details on the idea there would be interested in hearing them :) Perhaps a super high level example of what that would look like?

gauntface commented 8 years ago

Just started to muck around with this a bit more and ended up with the following.

In the layout file do something like:

{{#each inlineStyles}}
<style>
  {{{inlineFile this }}}
</style>
{{~/each}}

The inlineFile is a custom helper that is defined like so:

this._expressApp.engine('handlebars', exphbs({
  defaultLayout: 'default',
  layoutsDir: ....,
  partialsDir: ....,
  helpers: {
    inlineFile: function(filePath) {
      try {
        var actualFilePath = path.join(buildPath, filePath);
        return fs.readFileSync(actualFilePath);
      } catch (err) {
        console.log('Unknown file inline request.', filePath);
      }
      return '';
    }
  }
}));

This at least would make the file handling a little less cumbersome.

addyosmani commented 8 years ago

Thanks for digging into this, dude. If I'm reading the above right, we're iterating over each inlineStyles entry and outputting a new <style> tag for each one. Is this intentional? I was wondering if this might be better:

Check if there are any inlineStyles entries
If so, iterate over them from inside just one <style> block
Output all inline styles inside that single <style> block rather than creating a new block for each one

Any downsides to that approach?

jeffposnick commented 8 years ago

In case it helps to look at another implementation, here's the server-side code I'm using in my demo to inline styles for a given URL during the initial render. It reads in all the CSS contents to memory when the server starts up. I determine which style or styles are needed via a method on the component, needsStyles(), which is obviously very React-y.

// Startup
let styles = new Map(
  Object.keys(revManifest).filter(originalFile => originalFile.endsWith('.css'))
    .map(originalFile => {
      let revFile = revManifest[originalFile];
      let contents = fs.readFileSync(
        path.join('build', 'rev', revFile), 'utf8');
      return [originalFile, contents];
    })
);

// Inside the rendering function for a given component
let inlineStyles = renderProps.components
  .filter(component => component.needsStyles)
  .map(component => 'styles/' + component.needsStyles())
  .reduce((stylesSoFar, styleName) => {
    return stylesSoFar + styles.get(styleName);
  }, '');

// Render the template using inlineStyles

gauntface commented 8 years ago

cc @paullewis @addyosmani @PaulKinlan

I went a bit nuts and started to code up a different approach for application-shell which is in this branch: (https://github.com/GoogleChrome/application-shell/tree/new-approach)

This are one or two missing pieces to this, but it's hard to solve without a better example (perhaps we need to build a sample app with this and fill in the gaps).

Changes are chunked up below.

All server side endpoints are defined in app.js

See: https://github.com/GoogleChrome/application-shell/blob/new-approach/app.js

This means the single pathConfig file which stored endpoint with its style and script dependencies is dead. It also means the two separate controllers that serve the statically rendered version and the API version (or HTML partial) is gone.

PageOptions is a way to store css, js files that should be inlined or async'ly loaded in for an endpoint. (https://github.com/GoogleChrome/application-shell/blob/new-approach/server/models/page-options.js).

Serving Static + API Version

In app.js you'll notice there are calls like -> serverController.addUIEndpoint('/url-2', url2Options); This is a new function that will create the static and API endpoint. It takes in an express route (in this case '/url-2') and a PageOptions object which defines what the route should serve.

The method is as follows:

addUIEndpoint(route, pageOpts) {
  // Serve static render of page
  this._expressApp.get(route, (req, res) => {
    res.render(pageOpts.template, pageOpts.handlebarsConfig);
  });

  // Serve up a JSON response that can be used to load UI.
  this._expressApp.get('/api/partials' + route, (req, res) => {
    let templatePath = path.join(
      __dirname,
      '..', '..',
      'templates', 'views',
      pageOpts.template + '.handlebars'
    );
    this._handlerbarsInstance.render(
      templatePath,
      pageOpts.handlebarsConfig
    )
    .then(function(renderedTemplate) {
      var apiConfig = pageOpts.apiConfig;
      apiConfig.html = renderedTemplate;
      res.json(apiConfig);
    })
    .catch(function(err) {
      console.log('Partial API Error: ' + err);
      res.status(500).send();
    });
  });
}

It creates the get routes - one for the statically rendered version - res.render and one for the API version which returns JSON. This is beneficial since the two endpoints are generated with a single call - no duplication like we had before.

Missing Functionality: This needs to be defined up front which means adding dynamic data isn't possible. What could easily be done is for pageOptions to have an additional parameter which returns a promise which resolves with an object that will be handed to handlebars allowing dynamic content in both statically rendered and API endpoints. What would happen is on a new network request, the express callback would trigger the promise, get the data and then pass that to handlebars to use in it's rendering of the templates.

File Inlining in Templates

The templates have been changed to use the custom inlineFile tag.

{{#if inlineStylesheets}}
<style type="text/css">
{{#each inlineStylesheets}}
  {{{inlineFile this}}}
{{/each}}
</style>
{{/if}}

This is just neater than what we were doing before. Now we can define which files we want to inline anywhere and once its requested in the handlebars template, it'll be inlined in the final output.

ApplicationController

I made some hefty changes to the ApplicationController and how it's treated.

We originally had to define all Routes within the web app up front. This is no longer the case. Instead, calling loadNewPage() will make a call to the API for the supplied route (i.e. /api/partials) and if it's a successful request, the JSON response is parsed and the Styles and Javascript are loaded. After this the route is changed.

This part needs a lot more work. At the moment the use of the Router is circumvented.

What should be happening is we load styles, scripts and HTML partial but in a way that it has no affect on the current UI, in other words we hide the new UI. We would then register the new route with the router and call router.goToPath(pathName, responseObject.title);. This would go through the current state process where we would add in any animations out of the current UI and in of the new UI.

The only reason I haven't it is because I didn't want to invest further time if people think this is a dead end.

A more complex example would be beneficial here to ensure styles and scripts are loaded correctly and cleaned up correctly.

ApplicationController for Both Static and App Shell Renders

What I ended up finding was that loading the old JS we had for static pages as well as the "app-shell" model would cause issues with the navdrawer since they both tried to interact with it.

This made me realise that both static and app shell could use the ApplicationController. When defining JS to be loaded for a particular route, it should realistically only manipulate DOM that it has specific to it's page - NavDrawer is shared.

Both Static and App-Shell versions use 'ApplicationController' i.e. the AppShell and Static renders have ApplicationController. See: https://github.com/GoogleChrome/application-shell/blob/new-approach/templates/partials/close-page.handlebars#L29

This means that both now act as SinglePage web apps.

The only difference is that on an AppShell load (where the current UI and UI's styles and scripts would not be loaded), the ApplicationController will call loadNewPage for the current pathname, See: https://github.com/GoogleChrome/application-shell/blob/new-approach/frontend-assets/scripts/controller/ApplicationController.js#L30

That class name is set in the app-shell layout so won't kick in for static renders that have everything needed.

That's the major changes to this.

I did this largely to see what the end result would look like. I think it's a bit more palatable, what are your thoughts?

GoogleChromeLabs / application-shell