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: Added support for node v16 #1068

Merged
merged 1 commit into from
Mar 2, 2022
Merged

feat: Added support for node v16 #1068

merged 1 commit into from
Mar 2, 2022

Conversation

Jawayria
Copy link
Contributor

@Jawayria Jawayria commented Feb 3, 2022

JIRA: https://openedx.atlassian.net/browse/BOM-3260

Adds support for Node 16 and introduced some PropType changes to fix to fix eslint errors.

@netlify
Copy link

netlify bot commented Feb 3, 2022

✔️ Deploy Preview for paragon-openedx ready!

🔨 Explore the source changes: fdfdf5b

🔍 Inspect the deploy log: https://app.netlify.com/sites/paragon-openedx/deploys/621f7af704c3f80008a7744c

😎 Browse the preview: https://deploy-preview-1068--paragon-openedx.netlify.app

@netlify
Copy link

netlify bot commented Feb 3, 2022

✔️ Deploy Preview for paragon-edx ready!

🔨 Explore the source changes: fdfdf5b

🔍 Inspect the deploy log: https://app.netlify.com/sites/paragon-edx/deploys/621f7af7f8b2050008382a37

😎 Browse the preview: https://deploy-preview-1068--paragon-edx.netlify.app

@binodpant
Copy link
Contributor

@Jawayria you can update the snapshots and update the PR as per @adamstankiewicz we are okay to update those failed snapshots in the tests

you can run fedx-scripts jest --updateSnapshot or sth like it to update the jest snapshots and submit the changes

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

In addition to @binodpant's comment above, I added a nit comment.

GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
NPM_TOKEN: ${{ secrets.SEMANTIC_RELEASE_NPM_TOKEN }}
run: npx semantic-release --dry-run --branches=${{ steps.extract_branch.outputs.branch
}}
Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary newline? I believe }} could be on previous line?

package.json Outdated
@@ -44,7 +44,7 @@
"font-awesome": "^4.7.0",
"lodash.uniqby": "^4.7.0",
"mailto-link": "^1.0.0",
"prop-types": "^15.8.1",
"prop-types": "^15.7.2",
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this was downgraded?

@Jawayria
Copy link
Contributor Author

@Jawayria you can update the snapshots and update the PR as per @adamstankiewicz we are okay to update those failed snapshots in the tests

you can run fedx-scripts jest --updateSnapshot or sth like it to update the jest snapshots and submit the changes

Updated the snapshots. Now tests are passing with Node 12 and 14 but one test failure with Node 16

@binodpant
Copy link
Contributor

hmm I saw this same error in the explore catalog node16 node

I looked last week and have not figured out why yet... it seems there is an error about MutationObserver, which probably has changed api and some lib is still using it?

node:internal/process/promises:265
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "TypeError: MutationObserver is not a constructor".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

@binodpant
Copy link
Contributor

maybe this is useful: testing-library/dom-testing-library#477

@binodpant
Copy link
Contributor

actually for me it was due to a different reason but the same path of failures

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "Error: Warning: An update to %s inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

"jest": "^24.5.0",
"jest-cli": "^24.7.1",
"jest": "^25.0.0",
"jest-cli": "^25.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated jest version to fix TypeError: MutationObserver is not a constructor" when running tests with Node 16.
Reference

wrapper = mount(<Button
{...defaultProps}
/>);
/>, { attachTo: app });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest v25 uses jsdom v15 (link) and all the tests related to focus started to fail due to the changes introduced in jsdom v15 (link). So I have updated the tests and introduced attachTo in order to fix this issue following these references: 1 2

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM once we resolve the eslint issues by bringing the changes from here into this PR first.

When we merge this, I think we'll want the semantic-release type to be feat instead of build to denote via a new release version that Node 16 is now officially supported and so the changes to prop types are published as well.

@Jawayria Jawayria changed the title build: Added support for node v16 feat: Added support for node v16 Mar 2, 2022
@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #1068 (3d3d8a2) into master (5f9e3db) will decrease coverage by 0.27%.
The diff coverage is 33.33%.

❗ Current head 3d3d8a2 differs from pull request most recent head fdfdf5b. Consider uploading reports for the commit fdfdf5b to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1068      +/-   ##
==========================================
- Coverage   90.86%   90.59%   -0.28%     
==========================================
  Files         180      190      +10     
  Lines        2738     2989     +251     
  Branches      560      657      +97     
==========================================
+ Hits         2488     2708     +220     
- Misses        237      268      +31     
  Partials       13       13              
Impacted Files Coverage Δ
src/DataTable/CollapsibleButtonGroup.jsx 100.00% <ø> (ø)
src/DataTable/filters/CheckboxFilter.jsx 100.00% <ø> (ø)
...rc/DataTable/filters/MultiSelectDropdownFilter.jsx 100.00% <ø> (ø)
src/Icon/index.jsx 100.00% <ø> (ø)
src/InputSelect/index.jsx 12.50% <ø> (-0.55%) ⬇️
src/Modal/ModalDialogHeroBackground.jsx 100.00% <ø> (ø)
src/Modal/PopperElement.jsx 100.00% <ø> (ø)
src/Modal/index.jsx 92.50% <ø> (ø)
src/Overlay/index.jsx 100.00% <ø> (ø)
src/Popover/index.jsx 100.00% <ø> (ø)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f9e3db...fdfdf5b. Read the comment docs.

feat: Added support for Node 16
@Jawayria Jawayria merged commit a813309 into master Mar 2, 2022
@Jawayria Jawayria deleted the jawayria/node-16 branch March 2, 2022 14:25
@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 19.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants