-
Notifications
You must be signed in to change notification settings - Fork 246
chore: patch frontend-build deployment #1350
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
Conversation
@@ -84,6 +85,7 @@ | |||
"jest": "^26.6.3", | |||
"jest-console-group-reporter": "^1.0.1", | |||
"jest-when": "^3.6.0", | |||
"patch-package": "^8.0.0", |
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.
What is the intent of using this package for?
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.
patch-package
will try to read @openedx+frontend-build+13.0.30.patch
file after install. If you look at the file @openedx+frontend-build+13.0.30.patch
, it is just telling node_module/@openedx/frontend-build
to update specific file. Essentially the change for the that was just so webpack
would pick up JS_CONFIG_FILEPATH
environment variable
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.
Interesting; this does seem pretty complex though. What's the need for injecting this patch code here instead of updating the https://github.com/openedx/frontend-build package?
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.
NVM, I see that this is a temporary fix for that pr in the frontend-build like you mention in the PR description,
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1350 +/- ##
=======================================
Coverage 88.25% 88.25%
=======================================
Files 293 293
Lines 4997 4997
Branches 1267 1264 -3
=======================================
Hits 4410 4410
Misses 571 571
Partials 16 16 ☔ View full report in Codecov by Sentry. |
@leangseu-edx, we're probably going to have to remove this unless a compelling reason is found for this |
@leangseu-edx @arbrandes Is this patch still doing anything? I'd like to remove it, but it's unclear if it's still useful or not. |
@jsnwesson Do you know if this patch can be removed? |
@bradenmacdonald, we should create a PR to remove it and give 2U a couple of days to review it. Are you up for it, or should I? |
@arbrandes Sure! I've opened it: #1621 - but could you please help me get it reviewed? |
This pr is similar to openedx/frontend-app-learner-record#302
The can be remove once openedx/frontend-build#515 is merged