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.

Sign up for GitHub

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

Jump to bottom

File-loader: Output file-loader files in development #36337

Merged
merged 4 commits into from
Sep 26, 2019
Merged

File-loader: Output file-loader files in development #36337

merged 4 commits into from
Sep 26, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Sep 25, 2019 •
edited
Loading

Running npm start would result in file-loader requests 404ing. Reported in #36204 (comment).

This is due to the server bundler's webpack-dev-middleware, which hard codes a /calypso/evergreen/ public path.

This change updates the file-loader in development to match the expected path.

Testing instructions

@sirreal
Output file-loader files in development
bdbb0c1
@sirreal
Fix file-loader assets in development
3a4474e 
The fix for SSR file-loader URLs from #36204 caused a regression in
development. This was due to the way `publicPath` is hard-coded in the
server's webpack-dev-middleware.

When in that environment, use matching output and public path.
@sirreal sirreal requested a review from a team as a code owner September 25, 2019 21:05
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/file-loader-in-development

@sirreal sirreal requested a review from xknown September 25, 2019 21:08
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

sirreal
sirreal commented Sep 25, 2019
View reviewed changes
webpack.config.js
isDevelopment && ! isDesktop
? {
outputPath: 'images/',
publicPath: '/calypso/evergreen/images/',
Copy link
Member Author

@sirreal sirreal Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like the conditional configurations, but it seems necessary due to this:

wp-calypso/server/bundler/index.js

Line 104 in 53be70d

publicPath: '/calypso/evergreen/',

@sirreal sirreal self-assigned this Sep 25, 2019
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Defect labels Sep 25, 2019
@sirreal sirreal requested review from a team and sgomes September 26, 2019 05:59
tyxla
tyxla approved these changes Sep 26, 2019
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This LGTM 👍 Thanks for the fix!

@tyxla tyxla added [Status] Ready to Merge Build and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 26, 2019
jsnajdr
jsnajdr reviewed Sep 26, 2019
View reviewed changes
webpack.config.js
// Build off `output.path` for a result like `/…/public/evergreen/../images/`.
outputPath: path.join( '..', 'images' ),
publicPath: '/calypso/images/',
emitFile: browserslistEnv === defaultBrowserslistEnv, // Only output files once.
Copy link
Member

Choose a reason for hiding this comment

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

Was it intentional to remove the emitFile option? It prevents writing the same files twice when doing parallel evergreen/fallback builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did remove it intentionally because I had some concerns about the fragility of writing it once and hoping that the default always runs. I'm not sure how expensive these redundant writes are.

We can restore it if you prefer.

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 a bit worried about the files getting corrupted when two processes at once try to write them. No so much about performance really. I'd prefer to restore them. And always emitting in development, i.e., leaving the emitFile option undefined there.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Restored in e2c7c66

@jsnajdr
Copy link
Member

jsnajdr commented Sep 26, 2019

This is due to the server bundler's webpack-dev-middleware, which hard codes a /calypso/evergreen/ public path.

It took me a while to figure out what's going on here 🙂 In dev mode, the images are not written to disk: webpack-dev-middleware serves them from memory. It intercepts all requests to /calypso/evergreen/ subpaths and sends a response. The asset paths, however, are ../images/gridicons-xxx.svg. When writing to disk in production, that leads to the correct /calypso/images/... location. But in dev mode, the webpack-dev-middleware fails to intercept these requests, because the path prefix doesn't match its publicPath.

@sirreal sirreal requested a review from jsnajdr September 26, 2019 09:03
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Ready to Merge labels Sep 26, 2019
@jsnajdr
Copy link
Member

jsnajdr commented Sep 26, 2019

Pushed one more fix in e01cd6e:

  • configure the server-side file-loader, too, so that SSR-ed image URLs in development mode also have the /calypso/evergreen/images prefix. Without that, SSR-ed gridicons (like the "bug" icon in EnvironmentBadge point to invalid URLs
  • remove the isDesktop check when configuring client-side file-loader. That's not needed. The development modes for browser and desktop are identical in this regard.

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 26, 2019
@jsnajdr jsnajdr merged commit c158278 into master Sep 26, 2019
@jsnajdr jsnajdr deleted the fix/file-loader-in-development branch September 26, 2019 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyxla tyxla tyxla approved these changes

@xknown xknown Awaiting requested review from xknown

@sgomes sgomes Awaiting requested review from sgomes

@jsnajdr jsnajdr Awaiting requested review from jsnajdr

Assignees

@sirreal sirreal

Labels
Build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sirreal @matticbot @jsnajdr @tyxla