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.

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

[MLP-594] History #467

Merged
merged 27 commits into from
May 3, 2017
Merged

[MLP-594] History #467

merged 27 commits into from
May 3, 2017

Conversation

matteofigus
Copy link
Member

@matteofigus matteofigus commented Apr 19, 2017

This allows us to have an history of all the published components. In the UI, there is a new tab that allows to navigate the history, with all the components sorted by date.

A couple of details about how it works:

  • The complete history is saved in a new file hosted in the S3 bucket called components-details.json. In the future it may host much more than the publishedDate if necessary. That is a convenience view of the S3 components files tree.
  • When the registry starts and creates the components list cache, it verifies the integrity of the components details (by comparing the two) and takes care of, in case, building the up-to-date components-details file
  • The components details are not cached but just fetched when necessary
  • When a new component is published, both components list and details files will be updated.
  • The new history page can be quite huge in case of thousands of components and can benefit of a pagination component, not included in this PR.

For the review, I don't recommend going through all the commits - look at the diff and inline comments.

Home Page with 2 tabs:
screen shot 2017-04-26 at 13 59 29

History page:
screen shot 2017-04-26 at 13 59 46

@matteofigus matteofigus changed the title [WIP] History [MLP-594] History Apr 26, 2017

const save = (data, callback) => cdn.putFileContent(JSON.stringify(data), filePath(), true, callback);

const refresh = (componentsList, callback) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The refresh signature (called on registry start and component published) has the components list (all the components and all versions) as input. The reason is that both times this is needed to happen, it happens after the list is fetched or modified, so it is good to start from there for navigating the tree.

componentsCache.load((err, componentsList) => {
if(err){ return callback(err); }
componentsDetails.refresh(componentsList, (err) => callback(err, componentsList));
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what happens on registry start. In this scenario, the componentsDetails.refresh main goal is to verify integrity or to build the file from scratch (only the first time).

componentsCache.refresh((err, componentsList) => {
if(err){ return callback(err); }
componentsDetails.refresh(componentsList, callback);
});
Copy link
Member Author

@matteofigus matteofigus Apr 26, 2017

Choose a reason for hiding this comment

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

On component published, both files are in need to be updated and in need to succeed otherwise the publish will fail and the CLI will error

});
});
},
saveComponentsInfo: (componentsInfo, callback) => {
cdn.putFileContent(JSON.stringify(componentsInfo), `${conf.s3.componentsDir}/components.json`, true, callback);
Copy link
Member Author

Choose a reason for hiding this comment

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

this method had 0 usages so it was safe to be removed

@@ -7,7 +7,7 @@ const ComponentRoute = require('./routes/component');
const ComponentsRoute = require('./routes/components');
const ComponentInfoRoute = require('./routes/component-info');
const ComponentPreviewRoute = require('./routes/component-preview');
const ListComponentsRoute = require('./routes/list-components');
const IndexRoute = require('./routes/index');
Copy link
Member Author

Choose a reason for hiding this comment

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

The UI's "home page" used to be called "list components" route but now it's becoming much more than that. So list components => index

@@ -0,0 +1,28 @@
'use strict';
Copy link
Member Author

Choose a reason for hiding this comment

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

A little utility that given the details file maps to a convenient sorted array.

a(href="#components-history" class="tab-link") History

include components-list
include components-history
Copy link
Member Author

Choose a reason for hiding this comment

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

There are 2 jade partials, and via client-side javascript we toggle the selected one

@@ -3,7 +3,7 @@
const expect = require('chai').expect;
const sinon = require('sinon');

describe('registry : events-handler', () => {
describe('registry : domain : events-handler', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated, consistency renaming

@matteofigus matteofigus changed the title [MLP-594] History [wip] [MLP-594] History Apr 26, 2017
@matteofigus matteofigus changed the title [wip] [MLP-594] History [MLP-594] History Apr 26, 2017

const getFromDirectories = (options, callback) => {

const details = _.extend({}, _.cloneDeep(options.details));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why extending + deep cloning? (cloneDeep already return a new object if i'm not wrong)

Copy link
Member Author

@matteofigus matteofigus Apr 26, 2017

Choose a reason for hiding this comment

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

options.details can be empty in case it's the first time the new version is running with the current registry. So, options.details can be undefined and I need details to fallback to {} in that scenario. (I just tried to _.cloneDeep(undefined) and I got undefined back)

Copy link
Collaborator

@federicomaffei federicomaffei left a comment

Choose a reason for hiding this comment

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

Good stuff @matteofigus

@federicomaffei federicomaffei merged commit 5a840c2 into master May 3, 2017
@federicomaffei federicomaffei deleted the history branch May 3, 2017 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants