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.

Sign up for GitHub

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

Jump to bottom

revert: revert: "chore: upgrade to node 18" (#34496) #34503

Merged
merged 5 commits into from
Apr 23, 2024
Merged

revert: revert: "chore: upgrade to node 18" (#34496) #34503

merged 5 commits into from
Apr 23, 2024
+28,695 −33,564

Conversation

brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Apr 10, 2024 •
edited
Loading

  • Reverts commit eb26333. (Revert "chore: upgrade to node 18" #34496)
  • Fixes issue where built in xblock bundles were being prefixed with auto. For example, instead of VideoBlockDisplay.js we had autoVideoBlockDisplay.js
  • Fixes issues caused by simply removing string-replace-webpack-plugin without replacing it by using string-replace-loader
  • Fixes issue with xblocks not loading in studio by moving back to the previous version of the imports-loader webpack plugin and using string-based options configuration. Migrating to the new version can be done in a follow-up Upgrade webpack imports-loader and exports-loader plugins to v5 #34567

Tutor requirements

git+https://github.com/brian-smith-tcril/tutor.git@nightly-node18-patched
git+https://github.com/overhangio/tutor-mfe.git@nightly
git+https://gitlab.com/opencraft/dev/tutor-contrib-grove.git@main
git+https://github.com/hastexo/tutor-contrib-s3.git@main
git+https://github.com/overhangio/tutor-forum.git@nightly

@brian-smith-tcril brian-smith-tcril changed the title Reapply "chore: upgrade to node 18" (#34496) revert: revert: "chore: upgrade to node 18" (#34496) Apr 10, 2024
@brian-smith-tcril brian-smith-tcril force-pushed the revert-revert-node-18 branch 7 times, most recently from 2a1f3da to cd5ecaa Compare April 16, 2024 22:56
@brian-smith-tcril brian-smith-tcril added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Apr 18, 2024
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@xitij2000
Copy link
Contributor

@brian-smith-tcril
The issue wit the grove deployment might be because the only dependency listed is tutor itself. Could you append the other required plugins to the list to eliminate that as the cause of the issues?

git+https://github.com/overhangio/tutor-mfe.git@nightly
git+https://gitlab.com/opencraft/dev/tutor-contrib-grove.git@main
git+https://github.com/hastexo/tutor-contrib-s3.git@main

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@brian-smith-tcril
revert: revert: "chore: upgrade to node 18" (openedx#34496)
05baf4c 
This reverts commit eb26333.
@brian-smith-tcril
fix: don't prefix bundles with auto
28b4998
@brian-smith-tcril
fix: use string-replace-loader instead of `string-replace-webpack-p…
58185d7 
…lugin`
@brian-smith-tcril
chore: remove CommonsChunk plugin karma workaround
14b113c
@brian-smith-tcril
fix: move back to old version of imports-loader
27d33d4 
this fixes the issue with xblocks not loading in studio. it also requires use of the deprecated "string as loader options" method for imports and exports loader config
@brian-smith-tcril brian-smith-tcril marked this pull request as ready for review April 19, 2024 18:04
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

feanil
feanil reviewed Apr 23, 2024
View reviewed changes
webpack.common.config.js
{
// This file is used by both RequireJS and Webpack and depends on window globals
// This is a dirty hack and shouldn't be replicated for other files.
test: path.resolve(__dirname, 'cms/static/cms/js/main.js'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this section removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see this is not https://babel.pocoo.org/en/latest/ but https://babeljs.io/ and these pattern replacements are not necessary as the original patterns aren't in our codebase anymore and we don't want to add them back in.

@feanil feanil merged commit ff48125 into openedx:master Apr 23, 2024
66 checks passed
@kdmccormick kdmccormick mentioned this pull request Apr 23, 2024
Merged
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

kdmccormick added a commit to overhangio/tutor that referenced this pull request Apr 23, 2024
@kdmccormick
revert: revert: chore: upgrade to node 18 in edx-platform
21e32fc 
This reverts commit b1ffba2,
restoring the Node 16->18 upgrade.

Upstream PR: openedx/edx-platform#34503
This was referenced Apr 23, 2024
Merged
Closed
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil approved these changes

Assignees
No one assigned
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@brian-smith-tcril @open-craft-grove @xitij2000 @edx-pipeline-bot @feanil