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

JS_Pipeline and CSS_Pipeline hash error when using either roots-contentful or roots-records extensions #661

Open
markphillips100 opened this issue Aug 10, 2015 · 17 comments

Comments

@markphillips100
Copy link

Okay, so this is happening on Windows (10 but occurred on 8.1 too). I have no idea if the same problem exists on other platforms.

There are really two outcomes that occur when using the hash:true on each of the pipeline extensions and using either roots-contentful or roots-records configured to produce single-pages from data.

The first outcome
Functionally both dynamic content extensions work okay in that they produce pages based on a template and dynamic data sourced from the net. The issue stems from the compile timing of those generated pages and their use of a layout that uses != js() and != css() to render the outputs of js_pipeline and css_pipeline respectively.

Whilst normal jade templates referencing the layout will show hashed script and css file references, the single pages (from the dynamic extensions compilation) still show the non-hashed filename, as defined by the out parameter of the pipeline extension.

The second outcome
The hashing function in each pipeline method is executed multiple times resulting in the out file containing multiple hash numbers separated by a dot. E.g.:

build.d41d8cd98f00b204e9800998ecf8427e.d41d8cd98f00b204e9800998ecf8427e.b10659fe2ab28653f446f92b465cda00.css

This filename is the used in the non-dynamic pages like so:

<link rel='stylesheet' href='/css/build.d41d8cd98f00b204e9800998ecf8427e.d41d8cd98f00b204e9800998ecf8427e.b10659fe2ab28653f446f92b465cda00.css' />

This only occurs when using the dynamic content extensions and I am assuming because the outputs of each extension is not synchronized.

@markphillips100 markphillips100 changed the title JS_Pipeline and CSS_Pipeline hash error when using either roots-contentful or roots-records extentions JS_Pipeline and CSS_Pipeline hash error when using either roots-contentful or roots-records extensions Aug 10, 2015
@jescalan
Copy link
Owner

Hm, this is interesting. I can see why this has never been files before, usually hash is used only in app.production.coffee for roots compile commands, which are 1 shot and start fresh each time. Are you running hash: true while in development?

@markphillips100
Copy link
Author

I was testing a production compile. Emulating what would happen when my
site is generated during a production deploy on Azure website.
On 18 Aug 2015 10:57 pm, "Jeff Escalante" [email protected] wrote:

Hm, this is interesting. I can see why this has never been files before,
usually hash is used only in app.production.coffee for roots compile
commands, which are 1 shot and start fresh each time. Are you running hash:
true while in development?


Reply to this email directly or view it on GitHub
#661 (comment).

@jescalan
Copy link
Owner

Ok, I'll dig into this a little further today!

@shaekuronen
Copy link

Not sure if this is the same issue or not, my app.staging.coffee is

records = require 'roots-records'
sass = require 'node-sass'
js_pipeline = require 'js-pipeline'

module.exports =

    ignores: [
        '**/_*/**/*', '**/_*', '**/layout.*',
        'bower_components', 'bower_components/**/*',
        'data', 'data/**/*',
        '.gitignore', 'bower.json', 'readme.md',
        'circle.yml', '.bowerrc', 'README.html',
        'README.md'
    ]

    output: 'build'

    debug: true

    locals:
        images_path: '/img',
        css_path: '/css',
        js_path: '/js'

    server:
        clean_urls: true

    scss:
        sourceMap: true
        outFile: '/'

    'coffee-script':
        sourcemap: true

    extensions: [
        js_pipeline
            manifest: "assets/js/manifest.yml"
            out: 'js/main.js'
            minify: true
            hash: true
    ]

    jade:
        pretty: true

when run roots compile -e staging everything works except file revving. The main.HASH.js file is in the build dir, but is not referenced correctly in the rendered html.

The contents of foot.jade are

!= js()

this works just fine without hash option. But with hash option, the html is <script src='js/main.js'></script> instead of <script src='js/main.HASH.js'></script>

Tried removing the records require (which don't have configured yet anyway), but same behavior.

Currently using Roots 3.1.0 as per #655 there are some issues with latest release.

@kiejo
Copy link

kiejo commented Mar 18, 2016

Are there any updates or workarounds for this issue? I'm using roots 4.0.1 and roots-records 0.5.1 and this issue is preventing me from using this in production. Would be great to have a fix or workaround for this.

@jescalan
Copy link
Owner

Not at the moment, I haven't had a chance to fix it yet. I'll take a look as soon as I can.

Does this still happen if you just run roots compile once?

@kiejo
Copy link

kiejo commented Mar 18, 2016

Yes, this happens when I run roots compile using the following configuration. As I said this only happens in combination with roots-records.

js_pipeline(files: 'assets/js/**', out: 'js/build.js', minify: true, hash: true)
css_pipeline(files: 'assets/css/**', out: 'css/build.css', minify: true, hash: true)

Thanks for looking into this. I'm deploying to Netlify now which offers its own postprocessing options, which I'm using for the generation of hashes now. This allows me to set the hash option in roots to false as a workaround for this issue.

@davidmarquis
Copy link

davidmarquis commented Nov 10, 2016

I'm experiencing a very similar issue but with the use of the roots-i18n extension.

It's a very small site with only 2 pages. When I disable the roots-i18n extension, everything related to hashing works correctly. As soon as I enable the roots-i18n extension, the .js file reference in index.html gets properly revved, but the reference on the other page (business.html) does not get revved.

Worth noting that the issue seems to happen only with the last extension of either js or css listed in my array of extensions. If I put the js_pipeline after css_pipeline, the .js file does not get revved (hashed). If I put css_pipeline after js_pipeline, the .css file does not get revved (hashed).

The roots-i18n extension creates new additional output files for each language... my gut feeling is that it appears to be some sort of concurrency issue because some files are properly revved and some not. Is it possible that when an extension uses the write compile hook to emit new output files, the rest of the processing is negatively affected from a concurrency standpoint?

@jescalan
Copy link
Owner

I might cross-post this to roots-i18n? I didn't build this extension so I can't say I really am super familiar with its internals...

@davidmarquis
Copy link

davidmarquis commented Nov 10, 2016

It is quite simple, the relevant part seems to be this:

compile_hooks() {
      return { // TODO use async methods and return Promise

        // Called for every view template
        write: ({roots, file_options, content}) =>

          // Start iterating over our languages (as defined by the list of langFiles)
          // Return the translated content and the langFile path
          this.langFiles.map(langFile => [
            translate(content, langFile),
            langFile
          ]).map(([content, langFile]) => {
            const p = file_options._path,
                  dir = path.dirname(p),
                  ext = path.extname(p),
                  name = path.basename(p, ext),
                  lang = getLang(langFile)

            // Redirect Roots to write A FILE FOR EACH LANGUAGE, with 'en' as the default
            return {
              content,

              // Old code, may be useful later:
              //
              // path: path.join('.', dir, `${name}.${lang}${ext}`),
              // extension: `${lang}${ext}`

              path: lang === 'en' ?
                // Put English in the root output
                path.join('.', dir, `${name}${ext}`) :
                // And other languages in their subdirectories
                path.join('.', lang, dir, `${name}${ext}`),

              // Old code, may be useful later:
              //
              // path: path.join('.', dir, `${name}${ext}.${lang}`),
              // extension: `${ext}.${lang}`.substr(1)
            }
          })
      }
    }

For every language, the extension will create an object in the form {path: ..., content: ...} and return it. This seems to be in line with the description on Roots website: http://roots.cx/docs/extensions#write-hook

Nothing very esoteric... this observation combined with the other observations on the current issue seem to lead to something happening in Roots rather than in specific extensions.

@jescalan
Copy link
Owner

Thanks for the analysis here -- you definitely might be right. I don't really have the time to resolve this myself, but if you are willing to jump in and work on a patch I'd be happy to advise!

@davidmarquis
Copy link

davidmarquis commented Nov 10, 2016

I had a deeper look and I might have an idea of what's going on but still can't pinpoint where the exact origin would be.

Here's what I think is happening... maybe this combined with your 'insider look' might help progress the issue.

Looking at the code of roots-js-pipeline (roots-css-pipeline probably works very similarly), it is clear that the extension is applied while processing on source scripts (.js files)...

The bulk of the pipeline's processing when using minification and hashing is done in category_hooks

After combination and minification (a relatively lengthy process) the new file name is computed to add the MD5 hash. Only then is opts.out's value altered.

I believe the issue lies in the fact that the js() method that is made available to templates reads the opts.out too early in the process, maybe because views are rendered before the roots-js-pipeline extension has finished running.

Now the issue seems to show up only when we add specific extensions. These extensions are all extensions that seem to fiddle with views (roots-i18n, roots-contentful, etc.)... would using extensions that modify views allow the views to render before the CSS/JS pipelines have finished their lengthy process?

@davidmarquis
Copy link

davidmarquis commented Nov 10, 2016

Reading the documentation of the process_files method in compile.coffee (link) it seems that maybe a possible way to make all this work is to have roots-js-pipeline and roots-css-pipeline return ordered: true in their fs() methods?

That would break the benefits of parallel processing for simple cases so maybe their respective fs() implementations could inspect the user configuration, if hash is true, then they might return true for ordered... but that would arguably make things a little cryptic for future readers of that code.

I'll try to experiment with this avenue and report back...

@davidmarquis
Copy link

OK I had consistent success with returning ordered: true in the fs() function of the roots-i18n extension. I'll probably fork that project and tweak it for now because I need to ship...

But I fear that the issue at hand might be uncovering somewhat of a design flaw in the way Roots processes files in parallel... Let me know your thoughts!

@jescalan
Copy link
Owner

@davidmarquis sounds good! I don't think it's really a significant issue, to be honest. The thing is, it's extremely rare that roots users actually use the hashing, since deployment services like netlify, which are entirely free and interop very fluidly with roots, handle this for you. Roots has been stable for years, and used to build thousands of sites, and while a small number of people have run into this hashing issue, they all typically solve it by flipping off hashing and using netlify for hosting.

That being said, roots at the moment is on a feature freeze in favor of spike, which we are using and actively developing going forward. So if you have been able to work out this issue, I think it's ok going forward.

That being said, really appreciate the super thorough bug report and subsequent investigation -- not many people are willing to go that far and typically just ask me to solve the problem for them, so I really appreciate your taking the initiative!

@davidmarquis
Copy link

Ah crap, had I known that spike existed I would've used it for my project! :) I'll have a look...

Happy to help!

@jescalan
Copy link
Owner

@davidmarquis ah yeah we haven't really pushed any publicity on it yet, but we will sometime soon! it's still production ready and widely in use 😉

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

No branches or pull requests

5 participants