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

fix: Upgrade npm package @edx/studio-frontend to ^1.19.1 to fix missing assets #30309

Merged

Conversation

uetuluk
Copy link

@uetuluk uetuluk commented Apr 25, 2022

Description

The @edx/studio-frontend package is a requirement of edx-platform, with version ^1.17.0, which basically mean “anything between 1.0.0 (included) and 2.0.0 (excluded)” (see: https://semver.npmjs.com/).

The 1.18.0 update to the @edx/studio-frontend npm package breaks the current asset collection flow causing the following assets listed in the Supporting Information section to be missing from the Open edX images.

This Pull Request hopes to solve this issue by pinning @edx/studio-frontend package to 1.17.0.

Supporting information

The first version in which the dist/ folder was missing is 1.18.0: https://registry.npmjs.org/@edx/studio-frontend/-/studio-frontend-1.18.0.tgz Compare with 1.17.0 where the dist/ foldr was present: https://registry.npmjs.org/@edx/studio-frontend/-/studio-frontend-1.17.0.tgz

The following assets are missing:

  • static/studio/common/css/vendor/common.min.css
  • static/studio/common/css/vendor/courseOutlineHealthCheck.min.css
  • static/studio/common/css/vendor/assets.min.css
  • static/studio/common/js/vendor/runtime.min.js
  • static/studio/common/js/vendor/common.min.js
  • static/studio/common/js/vendor/courseOutlineHealthCheck.min.js
  • static/studio/common/js/vendor/assets.min.js

The discussion for this issue can be found here at the Tutor Discussion forum: https://discuss.overhang.io/t/missing-js-css-files-missing-from-openedx-docker-image-in-studio/2629/18

Testing instructions

The missing assets can be checked by running:

find node_modules/@edx/studio-frontend/ -name "common*.css"
node_modules/@edx/studio-frontend/dist/common.min.js

Thank you @regisb for organizing this information.

* fix: pin npm package @edx/studio-frontend to 1.17.0 to fix missing assets issue
@openedx-webhooks
Copy link

openedx-webhooks commented Apr 25, 2022

Thanks for the pull request, @uetuluk! I've created OSPR-6636 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@ghassanmas
Copy link
Member

@Jawayria Tagging you here since you seems to be done most commits for 18 release, may be somehting has to do with node 16 support?

ghassanmas added a commit to ghassanmas/studio-frontend that referenced this pull request Apr 28, 2022
  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.
@regisb
Copy link
Contributor

regisb commented Apr 29, 2022

FYI @uetuluk we're talking about you and your fix in the BTR meeting today that's happening in Lisbon, and we are working to make sure that it's merged as soon as possible. We really need you to sign the CLA in order to merge it. Did you already send it? (cc @nedbat @natabene who can maybe help you with this?)

@uetuluk
Copy link
Author

uetuluk commented Apr 29, 2022

I have already submitted the form, however, I did not receive the CLA to submit.

@natabene
Copy link
Contributor

@uetuluk Thanks for submitting the form, it is the same thing as CLA. Once the legal team processes it, I will post here.

Copy link
Contributor

@davidjoy davidjoy left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, though I'd like to understand why 1.18.0 is broken. Is fixing forward the right move, perhaps? Or do we know that 1.17.0 is what was tagged for Maple? If the latter, great, sounds good, let's merge this.

I'm on PTO next week and will be unable to follow up/stay in the loop. For that reason, can we get a CC to merge this and ensure it fixes the issues in Tutor? @regisb, are you or someone else from BTR able to shepherd this? I'll approve but won't be merging. Attn: @natabene, who tagged me!

@davidjoy
Copy link
Contributor

Also there's a broken check.

@ghassanmas
Copy link
Member

@davidjoy 1.18.1 is broken due to incorrect babel configuration, check this for more context openedx/studio-frontend#341

@regisb
Copy link
Contributor

regisb commented Apr 30, 2022

Thanks a lot to you @ghassanmas for the thorough explanation: https://discuss.openedx.org/t/buid-test-release-birds-of-feather-notes-lisbon-2022/7133/3 @pshiu you will want to read this!

@pshiu
Copy link
Contributor

pshiu commented Apr 30, 2022

Very interesting - thanks @regisb! And @ghassanmas, great fix!

regisb added a commit to overhangio/tutor that referenced this pull request May 6, 2022
…rontend requirement (see [discussion](https://discuss.overhang.io/t/missing-js-css-files-missing-from-openedx-docker-image-in-studio/2629) and [pull request](openedx/edx-platform#30309)) (by @regisb. Thanks @uetuluk!).

- [Fix] "The Compose file is invalid" error on mounting dev-only
  folders. (by @regisb)
- [Fix] CMS settings in development. (by @regisb)
ghassanmas added a commit to ghassanmas/studio-frontend that referenced this pull request May 12, 2022
  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 pushed a commit to openedx/studio-frontend that referenced this pull request May 13, 2022
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.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.
@davidjoy
Copy link
Contributor

Looks like we have a broken quality check; I can't quite tell what's wrong with it.

@erickhgm
Copy link

@uetuluk Can you update the studio-frontend version?

Does someone know the problem with CI / Quality Others (ubuntu-20.04, 3.8, 12) (pull_request)?

@ghassanmas
Copy link
Member

If I am not mistaken there is a problem with the test itself I got same error two months ago https://github.com/openedx/edx-platform/runs/5467542994?check_suite_focus=true for #29957 may be @rgraber knows about this or could confirm.

@uetuluk
Copy link
Author

uetuluk commented May 19, 2022

I upgraded the @edx/studio-frontend version to ^1.19.1 since the dist folder problem is solved with version 1.19.1.

@uetuluk uetuluk changed the title Pin npm package @edx/studio-frontend to 1.17.0 to fix missing assets Upgrade npm package @edx/studio-frontend to ^1.19.1 to fix missing assets May 19, 2022
package.json Outdated
@@ -7,7 +7,7 @@
"@edx/edx-proctoring": "^1.5.0",
"@edx/frontend-component-cookie-policy-banner": "1.0.0",
"@edx/paragon": "2.6.4",
"@edx/studio-frontend": "^1.17.0",
"@edx/studio-frontend": "1.17.0",

Choose a reason for hiding this comment

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

We have a new version 1.19.1 with the fix and other updates.

I've tested with maple.3 and tutor 13.2.2.

Suggested change
"@edx/studio-frontend": "1.17.0",
"@edx/studio-frontend": "1.19.1",

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I just committed the new version.

@uetuluk uetuluk requested a review from davidjoy May 19, 2022 02:53
@uetuluk
Copy link
Author

uetuluk commented May 20, 2022

Looks like we have a broken quality check; I can't quite tell what's wrong with it.

I saw that the check is failing because the package-lock.json is not updated.

https://github.com/openedx/edx-platform/runs/6514995518?check_suite_focus=true#step:10:72

I am not sure what is the Open edX way to generate the package-lock.json file.

@natabene
Copy link
Contributor

@uetuluk Thanks for submitting the forms, your request has been processed and your CLA check should clear now. Let me kick off the tests and see how it goes.

@davidjoy
Copy link
Contributor

If package-lock.json has gotten screwed up in some way (which can happen), usually I just delete it and run npm install to generate a new one. Not sure if that's what needs to happen here.

@uetuluk
Copy link
Author

uetuluk commented Jun 7, 2022

I rerun the npm install command to recreate the package-lock.json file as per the suggestion by @davidjoy. The command was run in the following environment:
node: 12.22.12
npm: 6.14.16

I have also locally run the scripts/xss-commit-linter.sh script that failed previously and it completed with 0 violations.

@davidjoy davidjoy changed the title Upgrade npm package @edx/studio-frontend to ^1.19.1 to fix missing assets fix: Upgrade npm package @edx/studio-frontend to ^1.19.1 to fix missing assets Jun 7, 2022
@davidjoy davidjoy merged commit b5e8532 into openedx:open-release/maple.master Jun 7, 2022
@openedx-webhooks
Copy link

@uetuluk 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Agrendalath pushed a commit to open-craft/edx-platform that referenced this pull request Jun 15, 2022
…ng assets (openedx#30309)

* fix: Pin npm package @edx/studio-frontend to 1.17.0

* fix: pin npm package @edx/studio-frontend to 1.17.0 to fix missing assets issue

* fix: Update npm package @edx/studio-frontend to 1.19.1

* fix: run npm install

* fix: recreate package-lock.json
Agrendalath pushed a commit to open-craft/edx-platform that referenced this pull request Jun 15, 2022
…ng assets (openedx#30309)

* fix: Pin npm package @edx/studio-frontend to 1.17.0

* fix: pin npm package @edx/studio-frontend to 1.17.0 to fix missing assets issue

* fix: Update npm package @edx/studio-frontend to 1.19.1

* fix: run npm install

* fix: recreate package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants