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

preload schemas #363

Merged
merged 4 commits into from
Jan 18, 2017
Merged

preload schemas #363

merged 4 commits into from
Jan 18, 2017

Conversation

nelsonpecora
Copy link
Contributor

  • pass schemas to _componentSchemas state property so they can be used by templates
  • allows kiln schema preloading

name: name,
schema: schema.getSchema(files.getComponentPath(name))
};
}));

Choose a reason for hiding this comment

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

Is kiln the only component interested in this data? I am wondering how this will affect page-load performance, as it increases the size of the state object. Perhaps there could be another end-point to return all the schemas which kiln could call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. It does increase the size of the state object, but not by a super huge amount. Alternatively, we could give a list of filepaths rather than loading the files, but that would require some odd kiln workarounds.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 680c5d4 on preload-schemas into f6d0194 on master.

if (fs.existsSync(path.join(dir, 'schema.yml'))) {
return path.join(dir, 'schema.yml');
} else if (fs.existsSync(path.join(dir, 'schema.yaml'))) {
return path.join(dir, 'schema.yaml');

Choose a reason for hiding this comment

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

Minor possible tweak: 'schema.yml' and 'schema.yaml' could be const at top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

@@ -94,6 +96,12 @@ function applyOptions(options, res) {

// can't assign to result, or we'd get a circular reference in refs.
_.set(state, '_components', componentList);
_.set(state, '_componentSchemas', _.map(componentList, function (name) {

Choose a reason for hiding this comment

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

_componentSchemaPaths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, I think this is better because they're not just an array of path strings (but rather objects with some properties)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4403aa4 on preload-schemas into f6d0194 on master.

@nelsonpecora nelsonpecora merged commit 6288d30 into master Jan 18, 2017
@nelsonpecora nelsonpecora deleted the preload-schemas branch January 18, 2017 22:28
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