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

Docs: Add recipe for static asset revisioning #2499

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silvenon
Copy link
Contributor

Even though this was not requested in #2164, it's a common practice, so a recipe would be nice. 😃

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

I'm not sure this recipe is useful. We shouldn't be showing people to write things to the filesystem, just to read them again and delete the originals.

Can you improve it?

docs/recipes/static-asset-revisioning.md Outdated Show resolved Hide resolved
docs/recipes/static-asset-revisioning.md Outdated Show resolved Hide resolved
@silvenon silvenon force-pushed the docs/recipe-asset-revisioning branch from 8d4d619 to d905115 Compare October 17, 2020 10:52
@silvenon silvenon force-pushed the docs/recipe-asset-revisioning branch from d905115 to 86859db Compare October 17, 2020 10:54
@silvenon
Copy link
Contributor Author

How about this approach?

@silvenon silvenon requested a review from phated October 17, 2020 10:55
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Better, but I think it still can be improved by not writing unnecessary steps to the filesystem.

}

function styles() {
return gulp
Copy link
Member

Choose a reason for hiding this comment

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

Let's use pipeline() (from node's Stream module) syntax any time things are being piped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you be more specific? I never used it before.

return del('dist/rev-manifest.json')
}

exports.dev = gulp.series(clean, gulp.parallel(styles, views), watch)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have a separate build and dev script. You are already vary on isProd which should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're removing watching (which I will), then it doesn't.

.src('views/**/*.njk')
.pipe(nunjucks.compile({ title: 'Hello world!' }))
// this updates the reference(s) to revisioned assets
.pipe(gulpIf(isProd, revRewrite({ manifest })))
Copy link
Member

Choose a reason for hiding this comment

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

Wow, this manifest option is a terrible API that's not using gulp correctly. I think you should combine views and styles tasks into 1 pipeline and use gulp-filter to manage the "phases".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll give it a shot.

Comment on lines +70 to +73
// after everything is done, we no longer need the manifest file
function deleteRevManifest() {
return del('dist/rev-manifest.json')
}
Copy link
Member

Choose a reason for hiding this comment

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

If you combine the 2 tasks, we no longer need to delete some intermediary file on the filesystem.

.pipe(gulp.dest('dist'))
}

function watch() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just remove the watch stuff, it doesn't add anything to this recipe.

Copy link
Contributor Author

@silvenon silvenon Oct 22, 2020

Choose a reason for hiding this comment

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

Also, nothing happens because I forgot to add tasks to them, so they are completely useless anyway 😆

function styles() {
return gulp
.src('styles/style.scss')
.pipe(sass().on('error', sass.logError))
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this pattern even makes sense anymore with pipeline()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to avoid this, too, but, as I said, I need help. 🙏

Comment on lines +35 to +37
function clean() {
return del('dist')
}
Copy link
Member

Choose a reason for hiding this comment

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

If you are using static revisioning, I'm pretty sure you're never supposed to remove your old files (in case someone loads an old version from their cache).

Copy link
Contributor Author

@silvenon silvenon Oct 22, 2020

Choose a reason for hiding this comment

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

Oh… I thought that if an old version is being loaded, then static revisioning isn't set up correctly because the cache was supposed to be busted. That's a terrifying thought for me as an author that someone loads an old CSS or JS file, I guess things will break for them either way? 🤔 Also, what about old pages? Isn't showing a new 404 page better than showing an old page that has been deleted in the source?

```json
{
"scripts": {
"dev": "gulp-dev",
Copy link
Member

Choose a reason for hiding this comment

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

If we just implement 1 task, this can be NODE_ENV=development gulp build

Comment on lines +76 to +77
// in build it's important that "views" runs AFTER "styles"
// if it runs before, the manifest file won't exist yet
Copy link
Member

Choose a reason for hiding this comment

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

If we change to my suggestions above, you don't need this not anymore since everything will be streaming within one task.

@silvenon
Copy link
Contributor Author

I'm learning (more) about streams in Node.js now so I can finish this PR. I actually never studied them before, I somehow managed to get by in gulp without it, but not having this theory established is becoming pretty uncomfortable. 😄

@silvenon
Copy link
Contributor Author

If you have some good resources, I'm all ears. 👂

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

Successfully merging this pull request may close these issues.

2 participants