Skip to content

Updating Yarn dependencies, making build process work again #857

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

Merged
merged 1 commit into from
Sep 27, 2018
Merged

Updating Yarn dependencies, making build process work again #857

merged 1 commit into from
Sep 27, 2018

Conversation

bobdenotter
Copy link
Contributor

Some housekeeping on the Yarn / Encore build process:

  • Updated symfony/encore to 0.20.1 from 0.9.1
  • Update a few other deps
  • Build process is now working again
  • Changed file naming strategy

Note: I replaced this

    .enableVersioning(false)

with this:

    .configureFilenames({
        js: '[name].js',
        images: 'images/[name].[ext]',
        fonts: 'fonts/[name].[ext]'
    })

I did this to keep the delta minimal. Since symfony/webpack-encore#110 .enableVersioning(false) is not doing what it says (for understandable reasons), but in this case I chose to keep the filenames without a versioning hash. If you think we should introduce filenames with version hashes instead, i'll happily update this PR.

@javiereguiluz
Copy link
Member

I'm not against replacing enableVersioning() by configureFilenames() ... as long as it's the recommended way in Webpack Encore and its docs. In this app we always do what Symfony Docs and Best Practices says. Thanks!

@bobdenotter
Copy link
Contributor Author

PR updated, Travis is green.

@javiereguiluz
Copy link
Member

@weaverryan quick ping: do you agree replacing enableVersioning() by configureFilenames() as a best practice in Webpack Encore? Thanks!

@weaverryan
Copy link
Member

Hi guys!

Hmm, I don't fully agree actually. If we were to have one "default" setting that we should encourage most people to use, it is to USE enableVersioning(). This is the default setting you get when you install the encore-pack, which works because the json_manifest_path is setup for you to read the manifest.json file. So, it's free versioning support.

Configuring the filenames so that the images do not include a hash is something that I think only a minority of users want. If you're using the json_manifest_path, then the image filename is read from the manifest.json file and is transformed into the versioned filename.

So actually, I think we should go the other direction - enable versioning :).

Cheers!

@weaverryan
Copy link
Member

(but I like all the other changes - things do need to be updated, from time to time)

@bobdenotter
Copy link
Contributor Author

I think we should go the other direction - enable versioning :).

That's fine with me, too. PR updated accordingly.

@javiereguiluz
Copy link
Member

Thanks Bob.

@javiereguiluz javiereguiluz merged commit 44d7267 into symfony:master Sep 27, 2018
javiereguiluz added a commit that referenced this pull request Sep 27, 2018
…n (bobdenotter)

This PR was merged into the master branch.

Discussion
----------

Updating Yarn dependencies, making build process work again

Some housekeeping on the Yarn / Encore build process:

 - Updated `symfony/encore` to `0.20.1` from `0.9.1`
 - Update a few other deps
 - Build process is now working again
 - Changed file naming strategy

Note: I replaced this

```
    .enableVersioning(false)
```

with this:

```
    .configureFilenames({
        js: '[name].js',
        images: 'images/[name].[ext]',
        fonts: 'fonts/[name].[ext]'
    })
```

I did this to keep the delta minimal. Since symfony/webpack-encore#110 `.enableVersioning(false)` is not doing what it says (for understandable reasons), but in this case I chose to keep the filenames without a versioning hash. If you think we should introduce filenames with version hashes instead, i'll happily update this PR.

Commits
-------

44d7267 Updating Yarn dependencies, making build process work again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants