-
Notifications
You must be signed in to change notification settings - Fork 55
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 webpack to version 5 #1201
Upgrade webpack to version 5 #1201
Conversation
27447f6
to
576299e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks for the detailed commit messages and setting up those example repos!
Upon running the build locally I do see webpack is emitting a warning from nodecomettransport.ts:275
. Looks like this is just caused by an errant Utils.inspect
which should be changed to util.inspect
. Other than that everything looks good to me 👍
And yes, you're right that the ES3 comments are outdated.
Thanks — I've created #1204 for this, since it seems like something that should be fixed on
OK — I'll make a PR to sort that out. |
Whilst working on the webpack 5 upgrade, it wasn’t clear to me which version of ECMAScript we’re actually targeting. But f2014f7 ("Update to ES5") and #839 ("Now we no longer advertise support for IE8 (or ES3)") suggested that we’ve dropped ES3 support. Owen subsequently confirmed this [1]. So let’s update the outdated references to ES3. [1] #1201 (comment)
Whilst working on the webpack 5 upgrade, it wasn’t clear to me which version of ECMAScript we’re actually targeting. But f2014f7 ("Update to ES5") and #839 ("Now we no longer advertise support for IE8 (or ES3)") suggested that we’ve dropped ES3 support. Owen subsequently confirmed this [1]. So let’s update the outdated references to ES3. [1] #1201 (comment)
The webpack v4 to v5 migration guide [1] says to "Upgrade webpack-cli to the latest available version". Presumably it means "the latest version compatible with webpack 4", which is this one [2]. Part of #1184. [1] https://webpack.js.org/migrate/5/#upgrade-webpack-4-and-its-pluginsloaders [2] https://github.com/webpack/webpack-cli/blob/master/CHANGELOG.md#500-2022-11-17
The webpack v4 to v5 migration guide [1] says to upgrade plugins to the latest version which supports webpack 4. 4.0.1 is the latest version of this plugin, and it appears to support both webpack 4 and 5. Part of #1184. [1] https://webpack.js.org/migrate/5/#upgrade-webpack-4-and-its-pluginsloaders
The webpack v4 to v5 migration guide [1] says to upgrade loaders to the latest version which supports webpack 4, which in the case of this loader is this version [2]. Part of #1184. [1] https://webpack.js.org/migrate/5/#upgrade-webpack-4-and-its-pluginsloaders [2] https://github.com/TypeStrong/ts-loader/blob/main/CHANGELOG.md#v900
It seems like we stopped using this in 2d792c3.
Part of our preparations for upgrading to webpack 5. 5.0.0 is the latest version of this package, and its README states that it supports both webpack 4 and 5. Part of #1184.
Ran the following: npm install webpack@latest \ webpack-cli@latest \ copy-webpack-plugin@latest \ tsconfig-paths-webpack-plugin@latest \ ts-loader@latest Then, performed the following changes to get the build green: - Configured webpack to emit ES5 code (`target` configuration) As described in [1], webpack 4 emitted ES5 code, but webpack 5 emits ES6 code by default. However, as documented in CONTRIBUTING.md, and as expected by `npm run check-closure-compiler`, we aim to use ES5 in the code we ship. So, configure webpack to emit ES5 code. - Removed config of Node-like polyfills (`node` configuration removed or replaced with `resolve.fallback`) As described in [2], webpack no longer automatically applies polyfills for Node.js APIs when targeting frontend environments. If you wish to apply polyfills, you need to opt-in though the the new `resolve.fallback` config. This means that we can remove the config telling it not to polyfill `Buffer`. As for the current behaviour of stubbing the Crypto module with an empty object, we achieve this with the `resolve.fallback.crypto: false` syntax which, whilst not properly documented in [3] at time of writing, is instructed by [4] ("If you are using something like node.fs: 'empty' replace it with resolve.fallback.fs: false") and described in an error message in the webpack code [5]. As Owen said in #1184 "As part of this work we will need to manually test that the lib still works in react-native and web workers, since these aren't covered by automated tests." I’ve tested this in a couple of small example apps [6] and [7], which just check that they’re able to import ably-js and publish / receive a message. [1] https://webpack.js.org/blog/2020-10-10-webpack-5-release/#improved-code-generation [2] https://webpack.js.org/blog/2020-10-10-webpack-5-release/#automatic-nodejs-polyfills-removed [3] https://webpack.js.org/configuration/resolve/#resolvefallback [4] https://webpack.js.org/migrate/5/#clean-up-configuration [5] https://github.com/webpack/webpack/blob/770a5a9cae8e2eddd5ca015efd06847e37480f45/lib/ModuleNotFoundError.js#L70-L72 [6] https://github.com/ably-labs/ably-js-react-native-example, commit 05a0177 [7] https://github.com/ably-labs/ably-js-web-worker-example, commit 62621db
576299e
to
c828952
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, feel free to merge once CI passes 👍
Resolves #1184. See commit messages for more details.
As specified by #1184, this targets the newly-created
integration/v2
branch.