-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix: empty build due babel incorrect configuration #341
Conversation
Thanks for the pull request, @ghassanmas! I've created OSPR-6648 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@Jawayria can you help @ghassanmas |
@ghassanmas Thank you for your contribution. @Jawayria Will you review this PR? |
Codecov Report
@@ Coverage Diff @@
## master #341 +/- ##
=======================================
Coverage 98.90% 98.90%
=======================================
Files 71 71
Lines 1641 1641
Branches 404 404
=======================================
Hits 1623 1623
Misses 18 18 Continue to review full report at Codecov.
|
package.json
Outdated
"babel-cli": "^6.26.0", | ||
"babel-eslint": "^7.2.3", | ||
"babel-jest": "^21.2.0", | ||
"babel-core": "^7.0.0-bridge.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.
We already have @babel/core
in dependencies then why do we need to add babel-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.
Yup good catch just removed it. May be was added by the script or by me while debugging and forgot to remove it... Anyway after removing still got the bundle when npm run build
@ghassanmas I enabled CI checks on this PR and looks like some configuration needs updates that you might have missed after replacing the outdated packages with newer packages |
The issue here is because we upgrade babel to v7 while using still using
So resolve I would need to revert this commit 8a14cdd. And that pacakge shall be removed when Footnotes |
Yeah thats okay with me, we can add this placeholder package until we don't upgrade babel-jest. We can also mention this thing in comments somewhere to keep it in records to remove that once the said upgrade is done |
The PR openedx#315 added @babel/core >= 7.*. upgrading/using babel v7 requires, making certian changes, e.g using @babel...etc. This lead to `npm run build` being interrupted/exited.without generating the /dist folder. Luckily, there is a pacakge to automate migration of config using `npx babel-upgrade --write`, followed by `npm install`, and few minor changes, the dist folder came to life again. This shall fixes this openedx/wg-build-test-release#166. and probably making this openedx/edx-platform#30309 unnecessary. Note: This change adds "^7.0.0-bridge.0" as devDep, this is needed because we have pacakges that uses Babel 6, which are jest and babel-jest. once we upgrade to babel-jest and jest >=24.*.* the pacakge shall be removed.
@aht007 can you rerun the tests now... I have readded the bridge pacakge... also wrote expalnation about it in the commit message |
@ghassanmas why can't we just upgrade babel-jest to v24 instead of going for a temporary solution? Is there any blocker on upgrading babel-jest to v24? |
@Jawayria two tests are failling:
I can fix them but I don't if I would be cheating or not. So I opted out from missing the test cases |
Oh alright. If it brings up test failures then leave the babel-jest upgrade. We can go with the current solution for now. |
@ghassanmas Are we good to merge this one now...? |
I am good but I can't merge it myself |
@ghassanmas 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
The PR #315 added @babel/core >= 7.*. upgrading/using babel v7
requires, making certian changes, e.g using @babel...etc. This
lead to
npm run build
being interrupted/exited.withoutgenerating the /dist folder.
Luckily, there is a pacakge to automate migration of config
using
npx babel-upgrade --write
, followed bynpm install
, andfew minor changes, the dist folder came to life again.
This shall fixes this openedx/wg-build-test-release#166. and
probably making this openedx/edx-platform#30309 unnecessary.