fix: order imports and remove circular dependencies #41824

ojeytonwilliams · 2021-04-14T14:56:21Z

The goal here was just to simplify the linting/formatting and order the the imports so that webpack would know how to order the css imports. This has worked, but we should audit the css, since the declaration order has probably changed.

However, reordering the imports showed that several modules had circular dependencies since they broke when the order changed. To fix this I separated out the offending code (types for actions) to flatten out the dependencies.

This did not fix redux/settings however, since there was a missing type, updateUserFlagComplete. I don't know how everything functioned without it, but I created it anyway.

ahmaxed · 2021-04-16T12:29:18Z

I didn't notice any inconsistencies in the UI. I am still investigating the action.

ahmaxed · 2021-04-17T08:19:21Z

So when you create an asyc type, the complete and error versions get created automatically.

freeCodeCamp/client/src/utils/createTypes.js

Lines 11 to 15 in e59ad6e

    
           export const createAsyncTypes = action => [ 
        
             `${action}`, 
        
             `${action}Complete`, 
        
             `${action}Error` 
        
           ];

ojeytonwilliams · 2021-04-17T09:37:20Z

That's interesting, thanks Ahmad. It explains why it wasn't completely broken! I will need to look into why the tests started failing, though.

ojeytonwilliams · 2021-04-22T09:30:38Z

@ahmadabdolsaheb mystery solved. You were spot on - there was no need to add updateUserFlagComplete. The reason the test was failing was another circular dependency issue: settingsTypes was undefined.

With that in mind, I think (short of introducing bugs) the only change this might have introduced is to the CSS. And that should be limited to the challenge pages.

gitpod-io · 2021-04-22T17:20:42Z

raisedadead · 2021-07-29T15:47:37Z

Do we want to give this one more go?

ojeytonwilliams · 2021-07-29T16:00:11Z

Sure. I'll take a stab at it tomorrow.

We might need a small freeze, though.

raisedadead · 2021-07-29T16:39:21Z

Sure.

redux depended on templates/Challenges/redux and vice versa. This meant that import order mattered and confusing bugs could arise. (cherry picked from commit 7d67a4e)

Import order generally does not matter, but there are edge cases (circular imports and css imports, for example) where changing order changes behaviour (cherry picked from commit b8d1393)

This brings the classic Show closer to the others as they now all create the description and title components

(cherry picked from commit 51a44ca)

(cherry picked from commit 25e2612)

gitpod-io · 2021-07-30T13:43:59Z

ojeytonwilliams · 2021-07-30T13:51:41Z

@raisedadead I removed the prettier changes and brought everything up-to-date. Ironically, ordering the inputs was not enough to get rid of these warnings:

chunk commons [mini-css-extract-plugin]
Conflicting order. Following module has been added:
 * css ./node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[1]!./node_modules/gatsby-plugin-postcss/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[2]!./src/t
emplates/Challenges/components/test-suite.css
despite it was not able to fulfill desired ordering with these modules:
 * css ./node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[1]!./node_modules/gatsby-plugin-postcss/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[2]!./src/t
emplates/Challenges/components/challenge-description.css
   - couldn't fulfill desired order of chunk group(s) component---src-templates-challenges-projects-backend-show-tsx
   - while fulfilling desired order of chunk group(s) component---src-templates-challenges-classic-show-tsx
 * css ./node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[1]!./node_modules/gatsby-plugin-postcss/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[2]!./node_
modules/prismjs/themes/prism.css
   - couldn't fulfill desired order of chunk group(s) component---src-templates-challenges-projects-backend-show-tsx
   - while fulfilling desired order of chunk group(s) component---src-templates-challenges-classic-show-tsx
 * css ./node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[1]!./node_modules/gatsby-plugin-postcss/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[9].oneOf[1].use[2]!./node_
modules/prismjs/plugins/line-numbers/prism-line-numbers.css
   - couldn't fulfill desired order of chunk group(s) component---src-templates-challenges-projects-backend-show-tsx
   - while fulfilling desired order of chunk group(s) component---src-templates-challenges-classic-show-tsx

because of nested imports. That's why I changed classic/Show.tsx slightly, so that it's closer to the other Show.tsxs as far as imports are concerned.

naomi-lgbt · 2021-07-30T14:50:36Z

There's a lot less noise when I run the development build now, but I do get this:

naomi-lgbt

Despite that warning, everything seems to look good locally. 🙂

ojeytonwilliams · 2021-07-30T16:11:59Z

Huh. It's weird that we don't have that rule. Let me see about adding that and fixing the issue.

naomi-lgbt

All better!

I still get this one, but I think this is actual babel and not us.

ojeytonwilliams · 2021-07-30T17:43:53Z

It's been around for a while, I think, https://github.com/freeCodeCamp/freeCodeCamp/runs/3204052148?check_suite_focus=true#step:9:221 and doesn't seem to be breaking anything

ahmaxed

Other than what Nick said, LGMT.

moT01

Well, I looked this over and tested it pretty thoroughly - didn't find any issue. LGTM 👍

* rename to TS * update import * migrate with-instant-search * change falsy values to empty string * update import order Finish resolving conflict from #41824 * update import order for search-bar * update setTimeout() type

ojeytonwilliams force-pushed the chore/simplify-prettier branch from 7cf87c1 to 25e2612 Compare April 14, 2021 17:08

ojeytonwilliams marked this pull request as ready for review April 22, 2021 17:45

ojeytonwilliams requested a review from a team as a code owner April 22, 2021 17:45

raisedadead added the status: merge conflict To be applied to PR's that have a merge conflict and need updating label Apr 26, 2021

ojeytonwilliams added 6 commits July 30, 2021 11:17

fix: remove circular dependency

cb9657d

redux depended on templates/Challenges/redux and vice versa. This meant that import order mattered and confusing bugs could arise. (cherry picked from commit 7d67a4e)

feat: require imports to be in alphabetical order

ceab9fc

Import order generally does not matter, but there are edge cases (circular imports and css imports, for example) where changing order changes behaviour (cherry picked from commit b8d1393)

chore: order imports

8be1ce7

fix: lift up challenge description + title comps

567708f

This brings the classic Show closer to the others as they now all create the description and title components

fix: remove donation-saga/index circular import

4fd7764

(cherry picked from commit 51a44ca)

refactor: extract action-types from settings

6c68517

(cherry picked from commit 25e2612)

ojeytonwilliams force-pushed the chore/simplify-prettier branch from 7dc4ad5 to 6c68517 Compare July 30, 2021 13:43

ojeytonwilliams changed the title ~~fix: order imports~~ fix: order imports and remove circular dependencies Jul 30, 2021

ojeytonwilliams removed the status: merge conflict To be applied to PR's that have a merge conflict and need updating label Jul 30, 2021

fix: lint errors

3896a77

naomi-lgbt previously approved these changes Jul 30, 2021

View reviewed changes

feat: prevent useless renames

0b626da

ojeytonwilliams dismissed naomi-lgbt’s stale review via 0b626da July 30, 2021 16:17

naomi-lgbt approved these changes Jul 30, 2021

View reviewed changes

ahmaxed approved these changes Aug 2, 2021

View reviewed changes

moT01 approved these changes Aug 2, 2021

View reviewed changes

moT01 merged commit e118dda into freeCodeCamp:main Aug 2, 2021

ojeytonwilliams deleted the chore/simplify-prettier branch August 2, 2021 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: order imports and remove circular dependencies #41824

fix: order imports and remove circular dependencies #41824

ojeytonwilliams commented Apr 14, 2021 •

edited

Loading

ahmaxed commented Apr 16, 2021

ahmaxed commented Apr 17, 2021 •

edited

Loading

ojeytonwilliams commented Apr 17, 2021

ojeytonwilliams commented Apr 22, 2021 •

edited

Loading

gitpod-io bot commented Apr 22, 2021

raisedadead commented Jul 29, 2021

ojeytonwilliams commented Jul 29, 2021

raisedadead commented Jul 29, 2021

gitpod-io bot commented Jul 30, 2021

ojeytonwilliams commented Jul 30, 2021

naomi-lgbt commented Jul 30, 2021

naomi-lgbt left a comment

ojeytonwilliams commented Jul 30, 2021

naomi-lgbt left a comment

ojeytonwilliams commented Jul 30, 2021

ahmaxed left a comment

moT01 left a comment

fix: order imports and remove circular dependencies #41824

fix: order imports and remove circular dependencies #41824

Conversation

ojeytonwilliams commented Apr 14, 2021 • edited Loading

ahmaxed commented Apr 16, 2021

ahmaxed commented Apr 17, 2021 • edited Loading

ojeytonwilliams commented Apr 17, 2021

ojeytonwilliams commented Apr 22, 2021 • edited Loading

gitpod-io bot commented Apr 22, 2021

raisedadead commented Jul 29, 2021

ojeytonwilliams commented Jul 29, 2021

raisedadead commented Jul 29, 2021

gitpod-io bot commented Jul 30, 2021

ojeytonwilliams commented Jul 30, 2021

naomi-lgbt commented Jul 30, 2021

naomi-lgbt left a comment

Choose a reason for hiding this comment

ojeytonwilliams commented Jul 30, 2021

naomi-lgbt left a comment

Choose a reason for hiding this comment

ojeytonwilliams commented Jul 30, 2021

ahmaxed left a comment

Choose a reason for hiding this comment

moT01 left a comment

Choose a reason for hiding this comment

ojeytonwilliams commented Apr 14, 2021 •

edited

Loading

ahmaxed commented Apr 17, 2021 •

edited

Loading

ojeytonwilliams commented Apr 22, 2021 •

edited

Loading