Content-Length: 566550 | pFad | https://github.com/namecheap/ilc/pull/209

D7 Fetch SPA bundle by oleh-momot · Pull Request #209 · namecheap/ilc · GitHub

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.

Sign up for GitHub

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

Jump to bottom

Fetch SPA bundle #209

Merged

oleh-momot merged 1 commit into master from feature/fetch-spa-bundle

Oct 9, 2020

Contributor

oleh-momot commented Oct 5, 2020

No description provided.

oleh-momot changed the title ~~Feature/fetch spa bundle~~ [Registry] Fetch SPA bundle

oleh-momot changed the title ~~[Registry] Fetch SPA bundle~~ Fetch SPA bundle

oleh-momot requested review from StyleT and nightnei

October 5, 2020 15:53

nightnei approved these changes

View reviewed changes

registry/server/apps/routes/createApp.ts Outdated

+                      try {
+                          response = await axios.get(app.assetsDiscoveryUrl, {responseType: 'json'});
+                      } catch (error) {
+                          console.log('Caught an error while trying to fetch a manifest file:');

Contributor

nightnei Oct 6, 2020

add link(app.assetsDiscoveryUrl) to the end of the console.log

Contributor Author

oleh-momot Oct 9, 2020

Done

registry/server/apps/routes/createApp.ts Outdated

+                          response = await axios.get(app.assetsDiscoveryUrl, {responseType: 'json'});
+                      } catch (error) {
+                          console.log('Caught an error while trying to fetch a manifest file:');
+                          console.log(error);

Contributor

nightnei Oct 6, 2020 •

edited

Loading

why do you use log instead error?
you can provide the second argument to the first console.log

Contributor Author

oleh-momot Oct 9, 2020

Done

StyleT reviewed

View reviewed changes

registry/server/apps/routes/createApp.ts Outdated

+                          app.cssBundle = processedManifest.cssBundle;
+                      }
+                      app.spaBundle = processedManifest.spaBundle;

Contributor

StyleT Oct 6, 2020

Here you also need to fetch dependencies from manifest

Contributor Author

oleh-momot Oct 9, 2020

Done

registry/server/apps/interfaces/index.ts Outdated

-                  spaBundle: commonApp.spaBundle.required(),
+                  spaBundle: Joi.when('assetsDiscoveryUrl', {
+                      is: commonApp.assetsDiscoveryUrl.required(),
+                      then: commonApp.spaBundle.optional(),

Contributor

StyleT Oct 6, 2020

Suggested change

      
                    then: commonApp.spaBundle.optional(),
          
                    then: commonApp.spaBundle.forbidden('You ca\'n specify... because ... data from assetsDiscoveryUrl will overwrite this anyway...'),

Contributor Author

oleh-momot Oct 9, 2020

I decided to leave optional here
It does not need to validate it, I guess

registry/client/src/apps/Edit.js Outdated

+                  return 'Trying to fetch the manifest file...';
+              };
               const InputForm = ({mode = 'edit', ...props}) => {

This comment was marked as outdated.

Sign in to view

registry/client/src/apps/Edit.js Outdated

                                   <AutocompleteArrayInput />
                               </ReferenceArrayInput>
                           </FormTab>
                           <FormTab label="Assets">
-                              <TextInput source="spaBundle" validate={required()} fullWidth />
-                              <TextInput source="cssBundle" fullWidth />
+                              <TextInput

Contributor

StyleT Oct 6, 2020

Disable this field when assetsDiscoveryUrl is specified

Contributor Author

oleh-momot Oct 9, 2020

Done

registry/client/src/apps/Edit.js Outdated

+                                  disabled={isFetchingManifest}
+                                  helperText={fetchingManifestHelperText}
+                              />
+                              <TextInput

Contributor

StyleT Oct 6, 2020

Before this field - add warning when assetsDiscoveryUrl is specified that values specified in these fields may be overwritten from manifest after saving.

Contributor Author

oleh-momot Oct 9, 2020

Added helper text for it

registry/client/src/apps/Edit.js Outdated

+                                              disabled={isFetchingManifest}
+                                              error={error}
+                                              helperText={helperText}
+                                              onBlur={(event) => {

Contributor

StyleT Oct 6, 2020

If you keep this logic - move it to async validator

Contributor Author

oleh-momot Oct 9, 2020

Removed

oleh-momot force-pushed the feature/fetch-spa-bundle branch from 9b6d57e to 1136cab Compare

October 6, 2020 14:58

oleh-momot requested a review from StyleT

October 7, 2020 16:32

StyleT suggested changes

View reviewed changes

Contributor

StyleT left a comment

This PR contains a lot of unrelated to the main task changes and so it can't be merged

registry/client/src/apps/Edit.js Outdated

+                  return `Do not need to specify SPA bundle, CSS bundle, dependencies because they would be fetched and set from assets discovery URL if they exist there`;
+              };
+              const validateAppCreation = (values) => {

Contributor

StyleT Oct 8, 2020

validateApp

Contributor

StyleT Oct 8, 2020

The code below is really complex. I don't really like the approach....

Contributor Author

oleh-momot Oct 9, 2020

Renamed
Removed unnecessary code but I left necessary validation here for 'assetsDiscoveryUrl', 'spaBundle'

registry/server/common/services/json.ts Outdated

@@ @@ -13,23 +13,23 @@ const isJSON = (str: string): boolean => { @@
                   return (/^[\],:{}\s]*$/).test(str);
               };
-              const parse = (value: any): any => {
+              const parseJSON = (value: any): any => {

Contributor

StyleT Oct 8, 2020

Why do you need to rename all these vars which unnecessarily cause a lot of side changes?

Contributor Author

oleh-momot Oct 9, 2020

Removed

registry/server/common/services/preProcessResponse.ts Outdated

		@@ -1,25 +1,23 @@
		import _ from 'lodash/fp';

Contributor

StyleT Oct 8, 2020

Why do you need to rename all these vars which unnecessarily cause a lot of side changes?

Contributor Author

oleh-momot Oct 9, 2020

Removed

registry/server/common/middlewares/validateRequest.ts Outdated

-                      res.status(422).send(preProcessErrorResponse(e));
+                  } catch (error) {
+                      console.error(`Caught an error while validating request data for '${req.url}':`, error);
+                      res.status(422).send(preProcessErrorResponse(new Error(preProcessError(error))));

Contributor

StyleT Oct 8, 2020

This line is super hard to understand....

Contributor Author

oleh-momot Oct 9, 2020

Removed

registry/server/errorHandler/preProcessErrorResponse.ts Outdated

		@@ -0,0 +1,10 @@
		const preProcessErrorResponse = (error: Error, errorInfo = {}) => {

Contributor

StyleT Oct 8, 2020

I don't see a big value in this function... IMO it causes more issues then it solves

Contributor Author

oleh-momot Oct 9, 2020

Removed

oleh-momot force-pushed the feature/fetch-spa-bundle branch 4 times, most recently from 45bfdf5 to b092e49 Compare

October 9, 2020 08:31


          Fetch SPA bundle when assets discovery url is provided

dced1c5

oleh-momot force-pushed the feature/fetch-spa-bundle branch from b092e49 to dced1c5 Compare

October 9, 2020 08:33

oleh-momot requested review from StyleT and nightnei

October 9, 2020 08:51

StyleT approved these changes

View reviewed changes

oleh-momot merged commit 8a7df26 into master

oleh-momot deleted the feature/fetch-spa-bundle branch

October 9, 2020 12:29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet