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

feat: update webpack config; HugoServer and Webpack Port to be same (3000) #1102

Merged
merged 23 commits into from
Mar 14, 2023

Conversation

ibrahimjaved12
Copy link
Contributor

@ibrahimjaved12 ibrahimjaved12 commented Mar 3, 2023

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop screenshots
    • Mobile width screenshots
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

#1096

What's this PR do?

This PR updates the webpack configuration for development such that the hugoServer and Webpack port is same(port 3000).
Previously hugoServer was at port 3000, and webpack port at 3001 which caused issues related to dynamic importing. It also introduces webpack watch mode for dev.

How should this be manually tested?

This PR requires to check that the structure is working as intended, and that all the modules used for building the website work fine.

Steps:

  1. Clear your browser cache
  2. Checkout to this branch.
  3. yarn fmt(check that its working)
  4. yarn start course and yarn start www
  5. Making sure ocw-www and ocw-course work as it was before.
  6. Making sure the webpack related modules/plugins are working i.e the assets, css styling, imports, js files, fonts, images, social media icons on footer, etc
  7. Check inspect element and ensure there are no errors related to this.
  8. Making sure lecture videos are playing on course.
  9. After disconnecting from server, do runyarn fmt again.

To test production webpack config:

  1. Clear your browser cache
  2. Checkout to this branch
  3. Open the file: package.json
    Modify line#10:
    From:
    "start:webpack": "yarn with-env --dev -- webpack --config ./base-theme/assets/webpack/webpack.dev.ts --stats-children",
    To:
    "start:webpack": "yarn with-env --dev -- webpack --config ./base-theme/assets/webpack/webpack.prod.ts --stats-children",
  4. Same above steps from 3-9.

@ibrahimjaved12 ibrahimjaved12 self-assigned this Mar 3, 2023
@ibrahimjaved12 ibrahimjaved12 changed the title Cc/ww feat: update webpack config; HugoServer and Webpack Port to be same (3000) Mar 3, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

Lighthouse results:

results for https://ocw-hugo-themes-www-pr-1102--ocw-next.netlify.app/:

Accessibility Best Practices Performance Progressive Web App SEO
100 💯 83 😄 51 😟 20 😱 79 🙂

results for https://ocw-hugo-themes-www-pr-1102--ocw-next.netlify.app/search/:

Accessibility Best Practices Performance Progressive Web App SEO
93 🎉 92 🎉 65 😐 30 😰 93 🎉

results for https://ocw-hugo-themes-course-v2-pr-1102--ocw-next.netlify.app/:

Accessibility Best Practices Performance Progressive Web App SEO
90 🎉 92 🎉 64 😐 30 😰 76 🙂

@ibrahimjaved12
Copy link
Contributor Author

ibrahimjaved12 commented Mar 6, 2023

As @ChristopherChudzicki and @gumaerc explained,
our playwright test case CI / test-e2e (push) is failing because the test case is run before the webpack build

image

Tried to run the tests on development build, but still failed:
package.json:
"build:webpack": "cross-env NODE_ENV=development webpack --config base-theme/assets/webpack/webpack.dev.ts",

  1. yarn build: webpack
    (the assets are now built to base-theme/static/static_shared/ as opposed to base-theme/dist/static_shared if we use the production details.
  2. tests-e2e/util/test_sites.ts
    LOCAL_OCW_PORT = 3000(previously 3010)
  3. yarn test:e2e --debug featured_courses.spec.ts
    So now we have our files serving at ..../static/static_shared/ (instead of ...../dist/static_shared/), on the same port,
    if i open my normal google chrome instance, everything seems to work:

image

However, on the chromium instance that is triggered by the test, it still fails:

image

image

Now from here, if I open another chromium window, or even an incognito one, it still fails. On the chromium instance the files are not existing on the eg.http://localhost:3000/static_shared/css/www.css path.

One thing that might have to be taken care is also port numbers: if we serve files parallel to playwright test, the served files need to be on the same port.

If we run npx serve and then run yarn test:e2e --debug featured_courses.spec.ts on the same port (3000) by changing tests-e2e/util/test_sites.ts, it doesn't allow and says port 3000 is already in use. By default it is set at 3010, and it looks for served files there but I believe the webpack build is not run at all at this point by the test

I also tried using concurrently with something like in package.json: "test:e2e": "concurrently \"yarn with-env --dev -- webpack serve\" \"sleep 5 && yarn with-env --dev -- playwright test\""
Our webpack serve is not configured,
image

Comment on lines 183 to 187
{
type: "redirect",
match: /^\/static_shared\//,
transform: url => `http://localhost:${env.WEBPACK_PORT}${url}`
transform: url => `http://localhost:$3000${url}`
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be deleted.

When the e2e tests run, the hugo output is served by a static file server using serve-handler. The static files have structure:

test-sites
|--tmp
   |--dist                                     <-- root directory of "overall" static site
      |--courses
         |--ocw-ci-test-course                 <-- hugo output of one course site 
         |--other-course-does-not-exist-yet    <-- hugo output of another course site
         |--could-add-more 
      |--ocw-ci-test-www                       <-- hugo output of root site, ocw-www

Let's say that the tests run on port 3010. Then previously requests to localhost:3010/static_shared/... had to be redirected to the webpack output. That's what line 185 was doing. We do not need that anymore.

(We do still need to redirect requests to localhost:3010/static_shared, since static_output is not at the root of the static site. But that's handled by OCW_WWW_REWRITE: all requests to localhost:3010/whatever are redirected to localhost:3010/ocw-ci-test-www/whatever, except when whatever = courses.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right - I was experimenting with stuff and I had put it back in. I removed it and that resolved the playwright test for dev side.

@ChristopherChudzicki
Copy link
Contributor

@ibrahimjaved12 Thanks for this comment. I suspect the reason you were seeing stuff work better in Chrome than in Chromium is that you probably had some files cached in Chrome. The Incognito Chrome vs Chromium behavior would be identical, I expect.

There is one more old webpack dev server thing we can get rid of, then the e2e tests should work with the dev build. See #1102 (comment)

We still need to resolve the dist/ vs static/ issue. I can think of two options:

  1. always use static/static_shared as the webpack output. I don't think we want this because I think every site would then copy the webpack output when building via chugo, which we've tried to avoid in the past. Don't want to copy webpack assets 2600 times when moving files to s3.
  2. always use dist/ as the webpack output (production and dev). Production will continue to work as it does now. To make dev work, we'd need to pass two hugo configs when running hugo. Hugo merges the configs. The additional new config would just have staticDir: - dist, so hugo would know to look there, too.
  3. Keep production building to dist, dev building to static, and use dev build during e2e tests. We were already using dev build during e2e tests. This would be simple, and avoids introducing an extra config.

Either (2) or (3) seems OK to me. (2) seems maybe slightly preferable, just since the webpack output goes to same location whether dev or prod. @gumaerc @ibrahimjaved12 what do you think?

@ibrahimjaved12
Copy link
Contributor Author

ibrahimjaved12 commented Mar 7, 2023

@ibrahimjaved12 Thanks for this comment. I suspect the reason you were seeing stuff work better in Chrome than in Chromium is that you probably had some files cached in Chrome. The Incognito Chrome vs Chromium behavior would be identical, I expect.

There is one more old webpack dev server thing we can get rid of, then the e2e tests should work with the dev build. See #1102 (comment)

We still need to resolve the dist/ vs static/ issue. I can think of two options:

  1. always use static/static_shared as the webpack output. I don't think we want this because I think every site would then copy the webpack output when building via chugo, which we've tried to avoid in the past. Don't want to copy webpack assets 2600 times when moving files to s3.
  2. always use dist/ as the webpack output (production and dev). Production will continue to work as it does now. To make dev work, we'd need to pass two hugo configs when running hugo. Hugo merges the configs. The additional new config would just have staticDir: - dist, so hugo would know to look there, too.
  3. Keep production building to dist, dev building to static, and use dev build during e2e tests. We were already using dev build during e2e tests. This would be simple, and avoids introducing an extra config.

Either (2) or (3) seems OK to me. (2) seems maybe slightly preferable, just since the webpack output goes to same location whether dev or prod. @gumaerc @ibrahimjaved12 what do you think?

I think I might be more inclined towards the (3) option, because of not having an extra config - and I have updated the PR with respect to that. - Is this the kind of implementation you had in mind?

But I leave the call for approach to you @ChristopherChudzicki and @gumaerc. With the (2) option, I'm mainly concerned with the necessary changes we would have to make in the depending files to include them both in order to merge them. I get a bit idea that we're adding staticDir - dist for hugo to know where to look for the served files - and we change output to dist/ from webpack (dev) for that matter. And if we include them both together - hugo will merge them.

But if we just add thestaticDir - distto the current config and the webpack output changes (static --> dist) to dev, what would be wrong with that? - like could you please enlighten to what's the need for another config file.

@ChristopherChudzicki
Copy link
Contributor

I think (3) is fine 👍


re

But if we just add the staticDir - dist to the current config and the webpack output changes (static --> dist) to dev, what would be wrong with that?

If we add staticDir: -dist to the production config (ocw-course-v2 in ocw-hugo-projects) then every time Hugo builds, it will copy the webpack output from dist to hugo's output, and that will get uploaded to s3, for every course. So, for 2600 courses.

On the live site, we want all 2600+1 (+1 = ocw-www) courses using the same webpack output. It saves on s3 uploads (quicker publish). But more importantly, it also means that users can cache the bundle between courses.

Copy link
Contributor

@fakhar-ud-din fakhar-ud-din left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ibrahimjaved12
Copy link
Contributor Author

ibrahimjaved12 commented Mar 8, 2023

Thanks @fakhar-ud-din

I'm putting this back to WIP because I found out the yarn fmt issue had been coming from this PR.
yarn fmt works before running yarn start www (or yarn start course) but after it does not. I think it is because fmt is also running inside the js files served by webpack, i.e static_shared/js/.....

Currently figuring this out.
Edit: Resolved

@ibrahimjaved12 ibrahimjaved12 removed the request for review from gumaerc March 8, 2023 12:02
Copy link
Contributor

@fakhar-ud-din fakhar-ud-din left a comment

Choose a reason for hiding this comment

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

Tested, working fine.
LGTM 👍

@ibrahimjaved12 ibrahimjaved12 requested a review from gumaerc March 9, 2023 14:03
@ibrahimjaved12 ibrahimjaved12 marked this pull request as ready for review March 13, 2023 13:21
Copy link
Collaborator

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

👍 LGTM as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants