-
Notifications
You must be signed in to change notification settings - Fork 47
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
Update all the things: frontend-base and frontend-build #275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #275 +/- ##
==========================================
+ Coverage 99.33% 99.45% +0.11%
==========================================
Files 63 61 -2
Lines 757 731 -26
Branches 113 112 -1
==========================================
- Hits 752 727 -25
+ Misses 5 4 -1
Continue to review full report at Codecov.
|
before_install: | ||
- npm install -g npm@latest | ||
- npm install -g greenkeeper-lockfile@1.14.0 | ||
- npm install -g npm@6 |
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.
This is unneeded, you'll get npm 6.11 with node 12. The whole before_install block can go away I think.
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.
Oh, nice.
<main> | ||
<Switch> | ||
<Route exact path="/" component={PaymentPage} /> | ||
<Route path="*" component={EcommerceRedirect} /> |
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.
Could we use <Redirect />
from react-router-dom?
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.
EcommerceRedirect
also logs... that said, it could internally use Redirect
.
I think I may leave it, just to minimize potential changes to the actual functionality.
@@ -1977,7 +1977,7 @@ exports[`<PaymentPage /> Renders correctly in various states should render all c | |||
/> | |||
<svg | |||
aria-hidden="true" | |||
className="svg-inline--fa fa-lock fa-w-14 lock-icon" | |||
className="svg-inline--fa fa-lock fa-w-14 fa-null fa-rotate-null fa-pull-null lock-icon" |
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.
This seems odd
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.
It's from updating font-awesome's libraries. They changed the classes they include on <FontAwesomeIcon />
, far as I can tell. The icons appear to still show up. ¯_(ツ)_/¯
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.
Kinda a reset, maybe?
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.
I guess it's a new, likely unexpected, thing: FortAwesome/react-fontawesome#289
// For every ajax response, check if the API has | ||
// responded with a redirect value. If so, redirect. | ||
/* istanbul ignore next */ | ||
apiClient.interceptors.response.use((response) => { |
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.
Is this now incorporated into frontend-base? I recall this behavior being new and unique to payment
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.
Nevermind I see this below
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.
👍
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.
Looks good to me with the exception of: https://github.com/edx/frontend-app-payment/pull/275/files#diff-0135cf2733403b932b0932386f98353e . Not sure where the nulls are coming from.
c75ec06
to
236637f
Compare
In anticipation of frontend-base and frontend-build taking over.
It’s only used there, so it makes sense for it to live with the code.
… in appropriate places to their consumers. The form input and select are in payment-form because that’s where they’re used. PageLoading is up top.
Renaming serviceUtils to handleRequestError since that’s the only thing in it.
Also doing the config in setupTest.js to remove it from most test files.
There are a few I didn’t put in this commit because they’re on lines with other, unrelated changes. Will merge them in next. This commit also includes cutting over to App.getQueryParams in the selectors as it was difficult to disentangle from removing the configuration portion of the redux store.
The whole app should now use App.config and App.apiClient. Deleting environment.js in this commit. This also ceases to call the “configureApiService” methods for most services except the top-level one, because it still has the interceptor.
Also cleaning up common seletors and actions. common/selectors were moved into the order-details/data/selectors where they were still necessary, and the common/actions file is subsumed into frontend-base.
It will be attached to the apiClient in the top-level index.jsx file, rather than the payment service.
- The snapshot change is because of the updated version of font awesome from a prior commit. - Changing its export from ConnectedPaymentPage to just PaymentPage. - The “requirePaymentPageProps” thing in the PaymentPage.test.js file wasn’t actually doing anything. At all.
It’s used to catch 404s (in the MFE) and forward them on to the ecommerce service. This will be used in our route configuration.
- Assets were not used by this MFE or are included in its dependencies. - Normalizing the stylesheet and preparing it for using the new header and footer. - Moving all the app-specific CSS into the payment/index.scss file - the top level one just glues everything together.
…longer responsible for creating a history object.
Depends on previous commits being in place. index.jsx simplifies massively and App.js (in this repo) is removed, along with the messages file which wasn’t being used. - loadConfig is used to add additional environment variables. - APP_AUTHENTICATED event is used to attach the interceptor.
And updating husky to its recommended configuration method.
Also using fedx-scripts for i18n extract.
- Also adding “bugs” to package.json and updating the package-lock. - The build.prod.js file has been renamed to create-apple-merchant-file and has been turned into a follow-up to the prod build, rather than a wrapper of it.
Using them in index.jsx instead of the old “App” file since that’s the first moment we’re about to start painting. Also updating references in OrderSummary and PaymentForm.
0ada010
to
8a54d40
Compare
So this looks big.
Review Method
Please review the commits individually - they have detailed commit messages that describe what they're doing. Note that the individual commits will not run or pass tests on their own. They do at the end - things were a bit too interconnected for me to both 1) keep the commits understandable and 2) keep them resulting in a working app.
High level overview
The goal here is to use frontend-base (application framework) and frontend-build (build scripts/config) in this repo. This involves:
Updating frontend-base
This has a few steps:
Updating frontend-build
Testing
All tests pass.
That said, we'll want to do a manual retest of the application in staging, and a smoke test of it in prod as well.