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

Stable webpacker 4.0 release #1823

Merged
merged 15 commits into from
Dec 14, 2018
Merged

Stable webpacker 4.0 release #1823

merged 15 commits into from
Dec 14, 2018

Conversation

gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Dec 2, 2018

This PR is final PR to finish up some remaining bits and release Webpacker 4.x.

Apologies it took us a while but looks like Webpacker is now in a great shape. Thanks, everyone for chipping in and making things better.

Things I would like to finish and ship:

Update: Will look into integrity hashes in other PR or perhaps after 4.0 release including engines support.

@gauravtiwari
Copy link
Member Author

Please feel free to link other issues you might think are important and we can work together to either ship them in the initial 4.x release or subsequent 4.x releases.

@gauravtiwari gauravtiwari added the WIP This PR is work in progress label Dec 2, 2018
@gauravtiwari gauravtiwari changed the title Stable webpacker 4.0 release Stable webpacker 4.0 release (WIP) Dec 2, 2018
@gauravtiwari
Copy link
Member Author

Is someone up for helping with docs i.e. if we need to improve or update? Feel free to make PRs

@gauravtiwari
Copy link
Member Author

Also, need to make a list of features that are new/improved in Webpacker 4.0

@gauravtiwari
Copy link
Member Author

Please expect the final release to be made by end of this week (once this PR is merged).

@ytbryan
Copy link
Contributor

ytbryan commented Dec 5, 2018

You will need yarn add postcss-flexbugs-fixes babel-plugin-macros to have an error free webpack-dev-server

3rd party webpacker libraries should note that #1822 is a breaking change.

@gauravtiwari
Copy link
Member Author

@ytbryan Both included with webpacker and works out of the box: https://github.com/rails/webpacker/blob/master/package.json#L27


chdirTestApp()

const config = require('../config')

describe('Config', () => {
beforeEach(() => jest.resetModules())
beforeEach(() => jest.resetModules() && resetEnv())
Copy link
Member

Choose a reason for hiding this comment

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

resetEnv like this might give you more problems than you would expect. I would do:

beforeEach(() => {
  jest.resetModules()
  process.env = {}
  // OR
  // delete process.env.RAILS_ENV
})

if (hasTrailingSlash) return path.substr(0, path.length - 1)
return path
}
const resetEnv = () => (process.env = {})
Copy link
Member

@jakeNiemiec jakeNiemiec Dec 10, 2018

Choose a reason for hiding this comment

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

https://travis-ci.org/rails/webpacker/jobs/465694990#L703 CI is pointing out that you are returning the value of the assignment. I would omit this altogether.

@yvbeek
Copy link
Contributor

yvbeek commented Dec 12, 2018

Thank you for all the great work and bringing webpack to rails.

Would it perhaps be a good default to rename app/javascripts to app/frontend? With all the styles, images and other resources it could be a bit easier to understand for new users.

I like the customizability of this framework but it is sometimes confusing which config to modify and where to inject, what, to make webpack behave a certain way.

Perhaps we could add a few predefined functions that you can import in the <environment>.js files to set-up common things like TypeScript / Sass loaders, Uglify options, Common chunks etc.

@pdfrod
Copy link

pdfrod commented Dec 12, 2018

Would it perhaps be a good default to rename app/javascripts to app/frontend? With all the styles, images and other resources it could be a bit easier to understand for new users.

This is precisely what I did on the project I'm working on - I got rid of app/javascripts and created an app/frontend folder. It didn't make sense to me still call it javascripts since it's becoming our go-to folder for all of our front-end assets (JavaScripts, CSS, images, etc...).

@yvbeek
Copy link
Contributor

yvbeek commented Dec 12, 2018

I can also recommend the following webpack dev server settings:

config/webpack/environment.js

const { environment } = require('@rails/webpacker')

// Reduce the output of the Webpack Dev Server
environment.config.merge({
  devServer: {
    stats: {
      entrypoints: false,
      errorDetails: false,
      modules: false,
      moduleTrace: false
    }
  }
});

module.exports = environment

Perhaps a good default? It reduces the output of webpack dev server to just the pack statistics (names, filesizes etc.).


module.exports = {
test: /\.(jpg|jpeg|png|gif|tiff|ico|svg|eot|otf|ttf|woff|woff2)$/i,
test: new RegExp(fileExtensions.join('|'), 'i'),
Copy link

@Morantron Morantron Dec 13, 2018

Choose a reason for hiding this comment

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

regexp should be changed slightly

Suggested change
test: new RegExp(fileExtensions.join('|'), 'i'),
test: new RegExp(`\\.(${fileExtensions.join('|')})$`, 'i'),

Copy link
Member Author

Choose a reason for hiding this comment

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

@Morantron Not sure if we need dot there since extensions already include them?

/.jpg|.jpeg|.png|.gif|.tiff|.ico|.svg|.eot|.otf|.ttf|.woff|.woff2/i

Copy link
Member Author

@gauravtiwari gauravtiwari Dec 14, 2018

Choose a reason for hiding this comment

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

Changed it to this:

new RegExp(`(${fileExtensions.join('|')})`, 'i') => 
/(.jpg|.jpeg|.png|.gif|.tiff|.ico|.svg|.eot|.otf|.ttf|.woff|.woff2)/i

Choose a reason for hiding this comment

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

True! It's missing the trailing dollar, It could be an issue since without it it can match the extension in the middle of the filename

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha thanks @Morantron 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #1832

@nickpith
Copy link
Contributor

@gauravtiwari I get the impression that this v4 release is getting held up so that things are in place for Rails 6 to make Webpacker the default asset pipeline. Is that correct?

That said, webpacker's lack of webpack v4 and babel v7 support is causing issues with us upgrading other dependencies that our applications use on the JS side like Storybook. It seems like the v4 release has been a long time in the release candidate state. It's ok if it will take some time to get things ready for Rails 6. I totally understand but is there a middle ground where consumers of Webpacker can utilize a stable version with webpack v4 and babel v7 support sooner rather than later?

@jakeNiemiec
Copy link
Member

@nickpith I think that was already merged into master rails/rails#33079 (rails/rails@4838c17#diff-e79a60dc6b85309ae70a6ea8261eaf95R37)

@gauravtiwari gauravtiwari changed the title Stable webpacker 4.0 release (WIP) Stable webpacker 4.0 release Dec 14, 2018
@gauravtiwari gauravtiwari removed the WIP This PR is work in progress label Dec 14, 2018
@gauravtiwari gauravtiwari merged commit e95dd72 into master Dec 14, 2018
@gauravtiwari gauravtiwari deleted the webpack branch December 14, 2018 20:24
@gauravtiwari
Copy link
Member Author

Published a release candidate, please give it a try and if no issues found, will make a final release this weekend.

@nickpith
Copy link
Contributor

Thanks @gauravtiwari!

@yvbeek
Copy link
Contributor

yvbeek commented Dec 14, 2018

I've added pull requests for:

Rename of app/javascript to app/frontend: #1833 (see comment)
Reduced webpack-dev-server output: #1834 (see comment)

@gauravtiwari
Copy link
Member Author

Made another release, which fixes a bug (#1835), thanks @Zyphrax for testing and reporting 🙏

@yvbeek
Copy link
Contributor

yvbeek commented Dec 16, 2018

Thanks @gauravtiwari, the bug has been fixed. Unfortunately the cacheGroups configuration seems to result in duplicate javascript imports in the final HTML. See #1835 for details.

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.

None yet

7 participants