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

Upgrade to webpack v5 #614

Merged
merged 3 commits into from
Jan 16, 2024
Merged

Upgrade to webpack v5 #614

merged 3 commits into from
Jan 16, 2024

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Nov 21, 2023

Upgrade to webpack v5

With manual testing to confirm these three UMD entry points still work:

import accessibleAutocomplete from 'accessible-autocomplete'

import Autocomplete from 'accessible-autocomplete/preact.js'
import Autocomplete from 'accessible-autocomplete/react.js'

Package audit

See results from npm audit below

6 vulnerabilities (3 moderate, 3 high)

To address all issues (including breaking changes), run:
  npm audit fix --force

@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from 9689056 to 2d24957 Compare November 21, 2023 09:44
@colinrotherham colinrotherham added the dependencies Pull requests that update a dependency file label Nov 21, 2023
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from 2d24957 to 97a49e9 Compare November 21, 2023 09:46
@colinrotherham colinrotherham force-pushed the package-updates branch 3 times, most recently from 267ed78 to b335388 Compare November 21, 2023 15:34
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from 97a49e9 to 7dfcb5a Compare November 21, 2023 16:37
@colinrotherham colinrotherham changed the title [WIP] Upgrade to webpack v5 Upgrade to webpack v5 Nov 21, 2023
@colinrotherham colinrotherham marked this pull request as ready for review November 21, 2023 16:44
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch 2 times, most recently from 64a41e7 to 5418dd5 Compare November 22, 2023 13:07
@colinrotherham colinrotherham self-assigned this Dec 4, 2023
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from 5418dd5 to a43a6f4 Compare December 4, 2023 19:57
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from a43a6f4 to 97d1576 Compare January 8, 2024 13:26
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from 97d1576 to cac257c Compare January 8, 2024 16:21
@romaricpascal romaricpascal added this to the Next milestone Jan 11, 2024
Base automatically changed from package-updates to main January 15, 2024 11:31
@romaricpascal romaricpascal force-pushed the package-updates-webpack branch 2 times, most recently from 90900ca to f0f41d3 Compare January 15, 2024 12:15
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.

Project built OK and accessible-autocomplete.min.js loaded OK in a browser.

Tried to load the library (using Node, for quickness) both with require and import which noticed that dist/accessible-autocomplete.min.js was adding to self rather than window as it does now, so we should probably go like-for-like (or even move to this as GOV.UK Frontend does?).


output: {
...config.output,
globalObject: 'window'
Copy link
Member

Choose a reason for hiding this comment

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

issue I think this one needs to go in the shared config's output, so that the wrapped version of the library also gets added to window, as it is with Webpack 4. With Webpack 5, looks like it gets added to self instead.

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, looks like on GOV.UK Frontend, we add to this rather than either of window or self. this sounds more portable, but:

  • isn't like for like with the current output of Webpack
  • I'm not quite sure if there'd be unforeseen side effects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romaricpascal Shall we pair on this?

You'll notice the difference when importing into the Review app

Copy link
Member

@romaricpascal romaricpascal Jan 16, 2024

Choose a reason for hiding this comment

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

During testing, sounds like a conflict happened if setting the accessible-autocomplete.min.js to 'window', so let's keep it on self.

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, it's when running without a bundler (e.g. dependencies via unpkg <script>)

It means we can use window.React.createElement() in user code but still ensure React inside our bundled library code still resolves and extends the window.React version using webpack globalObject

I'll add a comment to explain:

// Support extending `window.React` when not bundled
// e.g. with all dependencies included via unpkg.com
globalObject: 'window'

@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from f0f41d3 to e9a4bdd Compare January 16, 2024 11:16
@colinrotherham colinrotherham force-pushed the package-updates-webpack branch from e9a4bdd to ff3f5a6 Compare January 16, 2024 11:22
@romaricpascal
Copy link
Member

Let's just check that things still work serverside for React, as it was something specifically handled by this PR.

@colinrotherham
Copy link
Contributor Author

Let's just check that things still work serverside for React, as it was something specifically handled by this PR.

@romaricpascal Yep, all working as discussed on Slack

There's an extra commit 1746717 to fix server-side imports without breaking window.React

I've clarified why we use globalObject in #614 (comment) and have used this snippet to test a "string render" of the component via the React bundle both client-side and server-side:

import Autocomplete from 'accessible-autocomplete/react.js'
import React from 'react'
import ReactDOMServer from 'react-dom/server.js'

console.log(
  ReactDOMServer.renderToString(
    React.createElement(Autocomplete.default, {}, null)
  )
)

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.

Looks all good to go. Had a quick re-test through GOV.UK Frontend review app and it all worked fine.

@colinrotherham colinrotherham merged commit 4fabfb0 into main Jan 16, 2024
3 checks passed
@colinrotherham colinrotherham deleted the package-updates-webpack branch January 16, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants