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.

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
merged 1 commit into from
Oct 9, 2020
Merged

Fetch SPA bundle #209

merged 1 commit into from
Oct 9, 2020

Conversation

oleh-momot
Copy link
Contributor

No description provided.

@oleh-momot oleh-momot changed the title Feature/fetch spa bundle [Registry] Fetch SPA bundle Oct 5, 2020
@oleh-momot oleh-momot changed the title [Registry] Fetch SPA bundle Fetch SPA bundle Oct 5, 2020
nightnei
nightnei approved these changes Oct 6, 2020
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:');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

@nightnei nightnei Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

StyleT
StyleT reviewed Oct 6, 2020
View reviewed changes
registry/server/apps/routes/createApp.ts Outdated
app.cssBundle = processedManifest.cssBundle;
}

app.spaBundle = processedManifest.spaBundle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you also need to fetch dependencies from manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

registry/server/apps/interfaces/index.ts Outdated
spaBundle: commonApp.spaBundle.required(),
spaBundle: Joi.when('assetsDiscoveryUrl', {
is: commonApp.assetsDiscoveryUrl.required(),
then: commonApp.spaBundle.optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Disable this field when assetsDiscoveryUrl is specified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

registry/client/src/apps/Edit.js Outdated
disabled={isFetchingManifest}
helperText={fetchingManifestHelperText}
/>
<TextInput
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added helper text for it

registry/client/src/apps/Edit.js Outdated
disabled={isFetchingManifest}
error={error}
helperText={helperText}
onBlur={(event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you keep this logic - move it to async validator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@oleh-momot oleh-momot requested a review from StyleT October 7, 2020 16:32
StyleT
StyleT suggested changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@StyleT StyleT left a comment

Choose a reason for hiding this comment

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

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) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

validateApp

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

registry/server/common/services/preProcessResponse.ts Outdated
@@ -1,25 +1,23 @@
import _ from 'lodash/fp';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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))));
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is super hard to understand....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

registry/server/errorHandler/preProcessErrorResponse.ts Outdated
@@ -0,0 +1,10 @@
const preProcessErrorResponse = (error: Error, errorInfo = {}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@oleh-momot oleh-momot force-pushed the feature/fetch-spa-bundle branch 4 times, most recently from 45bfdf5 to b092e49 Compare October 9, 2020 08:31
@oleh-momot oleh-momot merged commit 8a7df26 into master Oct 9, 2020
@oleh-momot 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
Reviewers

@StyleT StyleT StyleT approved these changes

@nightnei nightnei Awaiting requested review from nightnei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oleh-momot @StyleT @nightnei