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

Fix webpack docs example #2869

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Fix webpack docs example #2869

merged 1 commit into from
Oct 19, 2022

Conversation

colinrotherham
Copy link
Contributor

Splitting out the "Fix webpack docs" commit from #2863 into this one

I've made these changes by hand so needs a bit of scrutiny

@colinrotherham colinrotherham added documentation User requests new documentation or improvements to existing documentation javascript squad labels Sep 21, 2022
@romaricpascal
Copy link
Member

Unrelated to the changes, but from giving it a quick go on my machine, I've had to make a few updates for npm install to run OK in the example folder:

  • update webpack's version to "^4.6.0" to not have conflicting versions
  • change node-sass to "sass": "^1.55.0" to have sass install OK with Node 16 (Node 14 was OK)

After that, however:

  • npm run dev doesn't boot a server at all and just exits
  • npm run build does build a main.js file in the public folder. Serving the folder with npx serve doesn't manage to load the styles (as it pulls them from ../../public/app.css

Looks like there's a few more things to do to get the Webpack example back on its feet (and maybe up to date with the latest Webpack versions?)

@colinrotherham
Copy link
Contributor Author

@romaricpascal Thanks for digging into this. I'll make it a draft as I only really fixed paths and named imports

@36degrees Perhaps we could add tests for various bundlers?

@colinrotherham colinrotherham marked this pull request as draft September 26, 2022 15:48
@36degrees
Copy link
Contributor

I've added this to the agenda to discuss at tomorrow's dev catch up.

@colinrotherham colinrotherham marked this pull request as ready for review September 29, 2022 09:26
@colinrotherham
Copy link
Contributor Author

@36degrees @romaricpascal I've also fixed webpack (including the CSS) ready for catch up

@colinrotherham colinrotherham force-pushed the fix-webpack-example branch 3 times, most recently from 343aac2 to 39c2725 Compare October 6, 2022 14:32
loader: 'sass-loader',
options: {
sassOptions: {
quietDeps: true
Copy link
Contributor Author

@colinrotherham colinrotherham Oct 6, 2022

Choose a reason for hiding this comment

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

@@ -7,6 +7,10 @@
"npm": "^8.5.0"
},
"license": "MIT",
"workspaces": [
"docs/examples/*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line makes sure npm install and npm ci also install webpack's dependencies

Copy link
Member

Choose a reason for hiding this comment

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

quibble Is that something we absolutely need for the example running? I'd rather we only brought example dependencies when trying to run the example rather than at every install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @romaricpascal

This was to stop the example(s) going stale or getting out of sync again.

Enables things like:

npm run dev --workspace ./docs/examples/webpack

It's possible of course, but then we'd need an extra lockfile (per example) and have to rely on file:// package paths to the parent govuk-frontend. We'd also opt out of:

  1. Package updates + auditing
  2. Dependabot dependency updates
  3. Automatic package deduplication
  4. Postinstall "npm ls --depth=0" check
  5. Example gets a symlink to govuk-frontend

I'd recommend the same approach for:

./app/package.json
./tasks/package.json

☝️ Not yet of course, but each "area" should manage its own packages and tests

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for clarifying 😄 I like the idea of properly separating the app and tasks as well, that sounds pretty neat 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 100%

Deployment to Heroku doesn't need all our developer/test tooling etc 😊

@@ -7,6 +7,10 @@
"npm": "^8.5.0"
},
"license": "MIT",
"workspaces": [
"docs/examples/*",
"package"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whilst ./package/package.json doesn't have any dependencies, it also creates a symbolic link from ./node_modules/govuk-frontend./package

This makes sure webpack's example can "find" the local govuk-frontend

import { Button } from 'govuk-frontend'

@colinrotherham colinrotherham force-pushed the fix-webpack-example branch 2 times, most recently from deefa44 to d5ed72c Compare October 11, 2022 10:54
@colinrotherham colinrotherham changed the base branch from main to babel-for-tests October 17, 2022 18:21
Base automatically changed from babel-for-tests to main October 18, 2022 14:23
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

The demo is working 🎉 Thanks for putting it back on its feet 🙌🏻

Only little quibble is with the installation for the examples dependency with the project's npm install (unless I misunderstood what it did), which I'd prefer to bring only when running the examples. It's not blocking for me if there's no other easy way 😄

@@ -7,6 +7,10 @@
"npm": "^8.5.0"
},
"license": "MIT",
"workspaces": [
"docs/examples/*",
Copy link
Member

Choose a reason for hiding this comment

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

quibble Is that something we absolutely need for the example running? I'd rather we only brought example dependencies when trying to run the example rather than at every install.

@colinrotherham
Copy link
Contributor Author

Thanks @romaricpascal

I'll link up to the reply (to your concern) as it's useful #2869 (comment)

@colinrotherham colinrotherham merged commit b4c5065 into main Oct 19, 2022
@colinrotherham colinrotherham deleted the fix-webpack-example branch October 19, 2022 08:50
@colinrotherham colinrotherham linked an issue Nov 29, 2022 that may be closed by this pull request
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation User requests new documentation or improvements to existing documentation
Projects
Development

Successfully merging this pull request may close these issues.

Split project into separate app, config, lib and tasks packages
3 participants