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

Integrated Webpack HTML Plugin. #1595

Merged
merged 23 commits into from
Sep 3, 2019
Merged

Integrated Webpack HTML Plugin. #1595

merged 23 commits into from
Sep 3, 2019

Conversation

revanth0212
Copy link
Contributor

Description

Added Webpack HTML Plugin to generate HTML at compile instead of runtime with UPWARD. HTML will be auto-generated using a template file that carries the boilerplate. In this case template.html.

Related Issue

Closes #1529.

Verification Steps

  1. Compilation should not fail and should generate index.html file in the dist folder inside venia-concept.
  2. When you go to any page on the UI and refresh, it should download the exact same index.html file irrespective of which page you are in. The previous implementation used to download a different file for different page.

Screenshots / Screen Captures (if appropriate)

Home Page:
image
Category Page:
image
Product Page:
image

Checklist

  • Functionality of Venia should not change. This is a compiler modification. A sanity test on the app should be fine.

@revanth0212 revanth0212 added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Aug 23, 2019
@revanth0212
Copy link
Contributor Author

Metrics coming soon.

revanth0212
revanth0212 commented Aug 23, 2019
View reviewed changes
packages/venia-concept/template.html Outdated
<head>
<meta charset="utf-8" />
<title><%= htmlWebpackPlugin.options.title %></title>
<meta name="viewport" content="width=device-width, initial-scale=1" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to my self: need to check the meta tags in prev templates and try getting them into either here or the app logic.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Aug 23, 2019 •
edited
Loading

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 9b0c74c

zetlen
zetlen previously requested changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

First of all, this is really impressive. It's a big speed boost. I'm excited for the change.

Second, thank you for investigating this so quickly and so well--it's a major change to the architecture and this is a bold move.

Third, I would love to see comparison screenshots or a link to a comparison test at webpagetest.org.

Fourth, I added a TODO comment in one place. Other than that, I think this is ready to merge, once other devs have taken a look at it.

packages/venia-concept/webpack.config.js Show resolved Hide resolved
@zetlen
Copy link
Contributor

zetlen commented Aug 26, 2019

Oh, I also would like to see as much as possible from open-document.mst implemented in the HTML template. At the very least:

  • The data-image-optimizing-origin and data-media-backend attributes on the <html> tag should be populated from the build environment
  • The static meta tags and link tags should be copied over
  • The web font should be copied over

@revanth0212
Copy link
Contributor Author

image

@revanth0212
Copy link
Contributor Author

Here are the results with changes: https://www.webpagetest.org/result/190826_KQ_b4d2b362747562e759bc975c5459d407/

@revanth0212
Copy link
Contributor Author

@zetlen 2 things,

  1. For some reason after switching to process.env.IMAGE_OPTIMIZING_ORIGIN, images are not loading up on CMS page. I suspect since IMAGE_OPTIMIZING_ORIGIN is not present in process.env.
  2. manifest.json always returns [Object: Object] an indication that the json object was not stringified properly.

Any thoughts on that?

supernova-at
supernova-at requested changes Aug 29, 2019
View reviewed changes
jest.config.js Show resolved Hide resolved
packages/venia-concept/src/MediaBackendURLFetcherPlugin.js Show resolved Hide resolved
packages/venia-concept/webpack.config.js Show resolved Hide resolved
packages/venia-concept/webpack.config.js Show resolved Hide resolved
packages/venia-ui/lib/components/App/app.js Show resolved Hide resolved
@revanth0212 revanth0212 mentioned this pull request Aug 29, 2019
Closed
@vercel vercel bot requested a deployment to staging August 29, 2019 16:36 Abandoned
@revanth0212 revanth0212 mentioned this pull request Aug 29, 2019
Closed
5 tasks
@vercel vercel bot requested a deployment to staging August 30, 2019 15:40 Abandoned
@jimbo jimbo dismissed zetlen’s stale review August 30, 2019 15:55

Andy says your changes were addressed.

@vercel vercel bot requested a deployment to staging September 3, 2019 16:13 Abandoned
@dpatil-magento dpatil-magento merged commit b35f92e into develop Sep 3, 2019
@revanth0212 revanth0212 deleted the webpackHtmlPluginIntegration branch September 14, 2020 21:48
@snyk-bot snyk-bot mentioned this pull request Apr 27, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soumya-ashok soumya-ashok soumya-ashok left review comments

@brendanfalkowski brendanfalkowski brendanfalkowski left review comments

@supernova-at supernova-at supernova-at approved these changes

@zetlen zetlen zetlen left review comments

Assignees

@zetlen zetlen

@supernova-at supernova-at

@soumya-ashok soumya-ashok

@revanth0212 revanth0212

Labels
pkg:pwa-buildpack pkg:venia-concept pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feature]: Add static, compiled SSR to Venia
7 participants
@revanth0212 @PWAStudioBot @zetlen @supernova-at @brendanfalkowski @soumya-ashok @dpatil-magento