build: fix peer depends and update common depends #615

ghassanmas · 2022-07-31T08:22:08Z

fix peer depends and update common depends

openedx-webhooks · 2022-07-31T08:22:11Z

Thanks for the pull request, @ghassanmas! 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.

natabene · 2022-08-02T14:25:15Z

@ghassanmas Thank you for your contribution, I kicked off the checks.

ghassanmas · 2022-08-02T17:13:59Z

@natabene Tests should be resolved now I have run them on the fork. Also apply for other PRs in profile and gradebook

codecov · 2022-08-02T17:55:51Z

Codecov Report

Base: 39.50% // Head: 39.50% // No change to project coverage 👍

Coverage data is based on head (b3f605a) compared to base (7a8ae85).
Patch has no changes to coverable lines.

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

Additional details and impacted files

@@                     Coverage Diff                     @@
##           open-release/nutmeg.master     #615   +/-   ##
===========================================================
  Coverage                       39.50%   39.50%           
===========================================================
  Files                             106      106           
  Lines                            2187     2187           
  Branches                          582      582           
===========================================================
  Hits                              864      864           
  Misses                           1238     1238           
  Partials                           85       85

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

timmc-edx · 2022-08-04T21:55:28Z

@davidjoy Would you be able to review this one, or point me to who should?

davidjoy · 2022-08-09T15:32:26Z

package.json

    "@fortawesome/fontawesome-svg-core": "1.2.36",
    "@fortawesome/free-brands-svg-icons": "5.15.4",
    "@fortawesome/free-regular-svg-icons": "5.15.4",
    "@fortawesome/free-solid-svg-icons": "5.15.4",
    "@fortawesome/react-fontawesome": "0.1.16",
    "@tensorflow-models/blazeface": "0.0.7",
-    "@tensorflow/tfjs-converter": "1.7.4",


This bumps these tensorflow libs through two major versions - do we know that we've absorbed the breaking changes in them and that this won't break?

This patch more or less a backport of 0527c73 and more of discussion can be found #611

These are not really used directly, I have researched the code and they are just there as peer depends of @tensorflow-models/blazeface 0.0.7 which require those exact versions hence. That being said, if something would break it would more likely to have broken before.

davidjoy

One change request and one sanity check on some of the version bumps.

davidjoy · 2022-08-09T15:34:04Z

package.json

@@ -91,5 +91,8 @@
    "react-test-renderer": "16.14.0",
    "reactifex": "1.1.1",
    "redux-mock-store": "1.5.4"
+  },
+    "peerDependencies": {


I don't think this is a meaningful change. frontend-app-account is not a package that gets installed as a dependency of anything else, and so can't have peer dependencies for a dependent to install. Further, we don't actually depend on bootstrap to my knowledge.

Paragon depends on it. And by default npm would install the latest version (i.e. boostrap is not pinned in paragon 19.14.1 . So If you try to install then build the app on a the latest npm 8 version. the build would would fail.
The point why I am using paragon 19.14.1 beacuse this the version frontend-app-learning is using, and the point is to have a consistant version of depends across MFEs that are used in openedx release.

Now the question is if frontend-app-learning (nutmeg master) is using paragon 19.14.1 why the build is not failing there? I can specualte that because at the time pacakge-lock.json was updated boostrap 4.6.1 was the latest. Or may be some other depends of learning require that exact version....

Anyway if you checkout to fronted-app-learning (nutmeg branch) amd run npm ls bootstrap you should get 4.6.1.

MFEs can't have peerDependencies - they're not a package that gets installed as a dependency. I don't know if somehow adding peerDependencies removes a warning or something, but I don't think that's the right way to solve whatever problem we're seeing.

It should not matter if MFEs have consistent versions of paragon - the code isn't shared anyway, I don't think it's meaningful to optimize for a consistent version. We shouldn't limit an MFEs version of a library because a different, independent MFE is at a particular version, that's not how the system was designed/intended to be used.

If we need to update these dependencies for nutmeg for some reason, we should just update them to latest - artificially pinning them at earlier versions is only going to cause problems and confusion later.

MFEs can't have peerDependencies - they're not a package that gets installed as a dependency. I don't know if somehow adding peerDependencies removes a warning or something, but I don't think that's the right way to solve whatever problem we're seeing.

The end goal is to force to use a spesfic verion of bootsrap, and I force to do so because otherwise it would break the build. I think it would be doable to have the same result if I instead moved it to deps instead of creating per depnds.

It should not matter if MFEs have consistent versions of paragon - the code isn't shared anyway, I don't think it's meaningful to optimize for a consistent version. We shouldn't limit an MFEs version of a library because a different, independent MFE is at a particular version, that's not how the system was designed/intended to be used.

Again the pinned vresion would resolve the failed build because the artificial created version of boostrap after updarading paragon will result in failed build. When I was debuggin this, I was confused, such that why the paragon update made the build to fail while the same version is being in learning and it builds okay. This made me further debugging the depends of paragon between the learning and other MFEs what I found is that boostrap version is different in the learning thus I tried pin boostrap version, rebuild again and the result was ok.

If we need to update these dependencies for nutmeg for some reason, we should just update them to latest - artificially pinning them at earlier versions is only going to cause problems and confusion later.

I agree that the right path would be frequently update depends for each MFE (Note: I am referring to the open release branches). The thing is its hard to know which version suppose to be compitable with open-release, so my stratgey is find the maximum version of all MFEs and consider that the right version.

Add to that for paragon for example the version is already pinned so artificially updaing the depends is not possible, a different startgey would be to go check the change-log of paragon version 19.* and find if there is any breaking changes and if not, then use the last minor version of paragon 19. which in this case is 19.25.3

So in summary; Assuming using paragon 19.25.3 wouldn't require us to pin boostrap and assuming MFEs would work as epxected using that version. Would you accept such change? (To MFEs nutmeg branch, learning, account profile and gradebook)

@davidjoy even using the latest minor version of paragon 19 didn't resolve it.
Anyway I stumbled upon this issue while checking paragon openedx/paragon/pull/1562 it seems the way to resolve to include the new varriable of bootsrap and if an MFE is using older paragon the solution was to pin it just as I did here ref commit in paryment mfe The difference is that they don't add it per rather direclty in depends.

However I went over npm docs, and the right way to do is to define overrides
I am now testing with overrides and see if build would success and then I willl deploy the same version in production to see if it would also would as expected

I tested and worked as expcted.
To test in production you can go: https://apps.edx.gsgapp.io/account you can use admin@zaat.dev and "zaatdev" as pass.

Bootsrap 4.6.2 cause build to fail due to missing variable: ref: openedx/paragon/pull/1562

abdullahwaheed · 2022-10-05T06:57:36Z

@ghassanmas there are some snapshots failing and local development server is not working with following error:

> @edx/frontend-app-account@1.0.0-semantically-released start
> fedx-scripts webpack-dev-server --progress

Running with resolved config:
/.../frontend-app-account/node_modules/@edx/frontend-build/config/webpack.dev.config.js

No local module configuration file found. This is fine.
1% setup initialize[webpack-cli] TypeError: cli.isMultipleCompiler is not a function
    at Command.<anonymous> (.../frontend-app-account/node_modules/@webpack-cli/serve/lib/index.js:146:35)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Promise.all (index 1)
    at async Command.<anonymous> (.../frontend-app-account/node_modules/webpack-cli/lib/webpack-cli.js:1672:7)

Please pull latest code from master, this error is due to old version of webpack, which is being updated in frontend-build.

ghassanmas · 2022-10-12T07:21:33Z

@abdullahwaheed just pushed a fix, by overrideing webpack-cli version hence webpack/webpack/issues/15951 . Also note this error already exists in nutmeg.master.

Update packages to remove security vulnerabilities (openedx#605) * fix: updated vulnerable packages * fix: fixed failed tests after package update * fix: linting issues failing ci tests * fix: package lock update * fix: snapshot updated to UTC * fix: missing dependency 'long'

ghassanmas · 2022-10-12T07:36:16Z

Secondaly for the tests I have backported 0527c73 and seems to resolve the failing tests

BilalQamar95 · 2022-10-13T10:17:28Z

@ghassanmas Are we sure that useEffect dependencies update won't cause unnecessary renders? If this was just to satisfy the linter I'd probably revert this and leave it as-is, ignoring the rule in case of useEffect Hook.

ghassanmas · 2022-10-13T10:54:03Z

@ghassanmas Are we sure that useEffect dependencies update won't cause unnecessary renders? If this was just to satisfy the linter I'd probably revert this and leave it as-is, ignoring the rule in case of useEffect Hook.

That chagne is backported to from 0527c73 to fix the failed tests.

arbrandes · 2022-10-31T14:49:14Z

Reviewers, how are we feeling about this? Any further changes requested? (@davidjoy, do you want to hand off final say to somebody else? Who owns the Account MFE, these days?)

arbrandes · 2022-10-31T14:50:31Z

Also, @ghassanmas, do you mind commenting on openedx/wg-frontend#140 so I can assign it to you? Or maybe you can assign yourself, not sure. :)

ghassanmas · 2022-11-01T13:23:43Z

@arbrandes I don't I can, but I have just comment there my perspective/finding on that.

mphilbrick211 · 2023-01-26T23:31:02Z

Hi @arbrandes and @ghassanmas! Just checking to see if this is still being pursued? If so, can it be reviewed/merged?

mphilbrick211 · 2023-02-07T20:46:21Z

@ghassanmas @arbrandes friendly ping on this :)

ghassanmas · 2023-02-08T12:43:53Z

@mphilbrick211 I think it's fine to close it, since I don't think we support nutmeg releases anymore.

mphilbrick211 · 2023-02-08T20:51:56Z

Ok, closing!

openedx-webhooks · 2023-02-08T20:52:01Z

@ghassanmas Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 31, 2022

build: fix peer depends and update common depends

5e5b8b0

ghassanmas force-pushed the nutmeg-depends branch from 2f734e3 to 5e5b8b0 Compare July 31, 2022 09:16

fix: removed es5 ci check

a841a3f

fix: pin boostrap to 4.6.1

b3f605a

davidjoy reviewed Aug 9, 2022

View reviewed changes

davidjoy suggested changes Aug 9, 2022

View reviewed changes

natabene assigned davidjoy Aug 9, 2022

ghassanmas requested a review from davidjoy August 17, 2022 11:18

jmakowski1123 mentioned this pull request Oct 31, 2022

Discovery: Bundle size is a blocker to builds openedx/wg-frontend#140

Closed

1 task

build: pin bootsrap and update paragon to latest minor version

ad0edf9

Bootsrap 4.6.2 cause build to fail due to missing variable: ref: openedx/paragon/pull/1562

fix: override webpack-cli version to resolve dev error

36a945c

ghassanmas force-pushed the nutmeg-depends branch from f52688a to b1c0124 Compare October 12, 2022 07:36

arbrandes assigned ghassanmas and unassigned davidjoy Nov 1, 2022

mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jan 26, 2023

mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 7, 2023

mphilbrick211 closed this Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: fix peer depends and update common depends #615

build: fix peer depends and update common depends #615

ghassanmas commented Jul 31, 2022

openedx-webhooks commented Jul 31, 2022

natabene commented Aug 2, 2022

ghassanmas commented Aug 2, 2022

codecov bot commented Aug 2, 2022 •

edited

Loading

timmc-edx commented Aug 4, 2022

davidjoy Aug 9, 2022

ghassanmas Aug 9, 2022

ghassanmas Aug 9, 2022

davidjoy left a comment

davidjoy Aug 9, 2022

ghassanmas Aug 9, 2022

ghassanmas Aug 9, 2022 •

edited

Loading

davidjoy Sep 2, 2022

ghassanmas Sep 6, 2022

ghassanmas Sep 6, 2022

ghassanmas Sep 6, 2022

abdullahwaheed commented Oct 5, 2022

ghassanmas commented Oct 12, 2022

ghassanmas commented Oct 12, 2022

BilalQamar95 commented Oct 13, 2022

ghassanmas commented Oct 13, 2022

arbrandes commented Oct 31, 2022

arbrandes commented Oct 31, 2022

ghassanmas commented Nov 1, 2022

mphilbrick211 commented Jan 26, 2023

mphilbrick211 commented Feb 7, 2023

ghassanmas commented Feb 8, 2023

mphilbrick211 commented Feb 8, 2023

openedx-webhooks commented Feb 8, 2023

build: fix peer depends and update common depends #615

build: fix peer depends and update common depends #615

Conversation

ghassanmas commented Jul 31, 2022

openedx-webhooks commented Jul 31, 2022

natabene commented Aug 2, 2022

ghassanmas commented Aug 2, 2022

codecov bot commented Aug 2, 2022 • edited Loading

Codecov Report

timmc-edx commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidjoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghassanmas Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdullahwaheed commented Oct 5, 2022

ghassanmas commented Oct 12, 2022

ghassanmas commented Oct 12, 2022

BilalQamar95 commented Oct 13, 2022

ghassanmas commented Oct 13, 2022

arbrandes commented Oct 31, 2022

arbrandes commented Oct 31, 2022

ghassanmas commented Nov 1, 2022

mphilbrick211 commented Jan 26, 2023

mphilbrick211 commented Feb 7, 2023

ghassanmas commented Feb 8, 2023

mphilbrick211 commented Feb 8, 2023

openedx-webhooks commented Feb 8, 2023

codecov bot commented Aug 2, 2022 •

edited

Loading

ghassanmas Aug 9, 2022 •

edited

Loading