-
Notifications
You must be signed in to change notification settings - Fork 1
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
Version 8.0 #581
Version 8.0 #581
Conversation
@@ -1,4 +1,7 @@ | |||
module.exports = { | |||
core: { |
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.
Needed to upgrade to webpack 5 in order to resolve issues with Node 18 and SSL. See: https://stackoverflow.com/questions/69692842/error-message-error0308010cdigital-envelope-routinesunsupported
@@ -9,7 +9,11 @@ module.exports = async ({ config }) => { | |||
// Storysource-addon | |||
config.module.rules.push({ | |||
test: /\.story\.jsx?$/, | |||
loaders: [require.resolve('@storybook/source-loader')], | |||
use: [ |
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.
Needed to change the webpack configuration in order to accommodate webpack 5.
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.
Can you see if a review app can be spun up for this PR? We may never had turned that on for this repo, but it would be nice to have.
.storybook/webpack.config.js
Outdated
@@ -39,5 +43,12 @@ module.exports = async ({ config }) => { | |||
src: path.resolve(__dirname, '../src'), | |||
}, | |||
} | |||
// Required for Node ^18.12.1 to resolve an OpenSSL configuraiton deprecation. |
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.
Typo: configuration
src/styles/modal.css
Outdated
@@ -0,0 +1,114 @@ | |||
body.modal-open { |
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.
Where did this file come from? Do consumers of this library need to reference this file in any way?
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.
Hmm. No idea. I certainly didn't add it intentionally. Removed...
migration-guides/v8.0.0.md
Outdated
|
||
Typically, the Node version of an application can be updated without any breaking change. However, Node 18 requires an upgraded version of `glibc`. This has impact on both Travis and the existing `node-sass` executable. | ||
|
||
For Travis, the default distribution used for builds and test execution does not have the version of `glibc` required by Node 18. To resolve this issue, a specific distribution is specified in the `.travis.yml` configuration file that has the correct `glibc` version. |
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.
Can you reference the distribution name directly in this paragraph please? Any official docs from Travis to reference for more info?
migration-guides/v8.0.0.md
Outdated
|
||
For Travis, the default distribution used for builds and test execution does not have the version of `glibc` required by Node 18. To resolve this issue, a specific distribution is specified in the `.travis.yml` configuration file that has the correct `glibc` version. | ||
|
||
`node-sass` has been deprecated and the latest version does not support the later `glibc` version (an exception is generated when one tries to execute `node-sass` with the later Node version). To resolve this, `node-sass` was removed and `sass` (Dart Sass, the currently active Sass implementation) was added. This change required an additional update to the `build:styles` script to use `sass` for style builds. |
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.
Let's add this paragraph to the section below.
migration-guides/v8.0.0.md
Outdated
|
||
## 2. Removed `node-sass` in favor of `sass` | ||
|
||
As mentioned above, `node-sass` was removed in favor of `sass`. Additionally, the latest version of `style-loader` was included in this version of `lp-components`. |
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.
Is the style-loader
note relevant in this section? Seems like your mention of it in #3 is sufficient enough IMO
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.
That's fine. Removed.
Would a review app do anything? There are no views in this file. There is supposed to be a storybook deployment to Heroku though. Does that no longer work? Or was the request for a "review app" storybook deployment? |
Exactly, I want to see the storybook deployment |
@chawes13 Cool. I don't see the |
@mwislek I just added you as a pipeline member. Let me know if that gives you enough permissions. It looks like the pipeline was disconnected after the Heroku security incident, which requires manual reconnection. |
@chawes13 Still don't see it. My permissions on the |
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.
Just a few non-blocking typos! This Node change being breaking is really annoying 😞 . I worry about getting buy-in from many existing apps to make this upgrade.
Maybe there's a best practice guide that could summarize / distill the work that you did in the client-template (since we don't have a migration guide concept there)?
@@ -1,5 +1,9 @@ | |||
<!-- Include description and link to resolved issue(s) here. --> | |||
|
|||
## Release Notes | |||
<!-- Optional description of the resolve issue(s) that will be included in the --> |
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.
Typo: resolved
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.
Oh man. Sorry about these.
@@ -1,5 +1,9 @@ | |||
<!-- Include description and link to resolved issue(s) here. --> | |||
|
|||
## Release Notes | |||
<!-- Optional description of the resolve issue(s) that will be included in the --> | |||
<!-- Github release description of this library. --> |
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.
Typo: GitHub
It's not in launchpadlab. It's in |
@chawes13 No email invitation. I don't even see the "Access" tab on |
Ok. I just changed your permissions, connected GitHub to the repo, and enabled review apps |
Would the "Release Notes" I added to this PR's description suffice or were you thinking of something new? I was assuming that this blurb about the Node version would be included in the release notes for all of the libraries that upgraded to Node 18. I included this same blurb in the client-template PR as well. |
webpack
,css-loader
,style-loader
, andsass-loader
to their later versions.Because there were significant changes in several of the library versions as well as a change to the
engines
configuration, this update is being treated as a major update.This work is being done as part of a client-template upgrade comparable to the update done for the Ionic client template.
Release Notes
This version contains the following breaking changes:
node-sass
in favor ofsass
Further explanation of each item is detailed below.
1. Upgraded Node version to 18.12.1
Typically, the Node version of an application can be updated without any breaking change. However, Node 18 requires an upgraded version of
glibc
. This has impact on Travis.The default distribution used for builds and test execution (
xenial
) does not have the version ofglibc
required by Node 18. See this Travis community forum note for a discussion of this issue. See this Node issue for a discussion of the newglibc
version requirement.To resolve this issue, a specific Linux distribution (
focal
) is specified in the.travis.yml
configuration file. This disribution has the correctglibc
version. See this Travis documentation for a description of thefocal
build environment.This version of
lp-components
will not support older Node versions. You must use Node version ^18.12.1.2. Removed
node-sass
in favor ofsass
node-sass
has been deprecated and the latest version does not support the laterglibc
version (an exception is generated when one tries to executenode-sass
with the later Node version). To resolve this,node-sass
was removed andsass
(Dart Sass, the currently active Sass implementation) was added. This change required an additional update to thebuild:styles
script to usesass
for style builds.3. Upgraded webpack and related loaders
The Node upgrade mentioned above also fixed an OpenSSL issue. That fix required that the latest version of Webpack be used, along with configuration of a specific hashing function. To accommodate this change, webpack was upgraded to ^5.75.0 and the
css-loader
,style-loader
, andsass-loader
were also upgraded to their latest versions.Author Checklist