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

fewer globals in app setup #1434

Merged
merged 4 commits into from
Oct 5, 2019
Merged

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Oct 5, 2019

This PR started as an experiment to see if we can move this chunk of code from scripts.html into the rest of the client-side JS:

<script src="//cdnjs.cloudflare.com/ajax/libs/showdown/1.3.0/showdown.min.js"></script>
<script type="text/javascript">
var files = {{ site.data.projects | jsonify }};
var projects = new Array();
var converter = new showdown.Converter();
for (var fileName in files) {
var project = files[fileName];
project.desc = converter.makeHtml(project.desc);
projects.push(project);
}
</script>

This code:

  • inlines all the site data on build as a JSON payload
  • downloads a third-party script (blocking)
  • reads all the projects and converts any markdown into HTML
  • sets projects as a global object

This is immediately used inside main.js as state for the ProjectsService:

var projectsSvc = new ProjectsService(projects),
compiledtemplateFn = null,
projectsPanel = null;

And projectsSvc is used in many places inside the script.

So before I look at moving that first snippet into the rest of the app, I wanted to tidy up how the app is composed using the projects global:

  • wrap it in a promise to emulate asynchronously loading the projects
  • explicitly pass in projectService wherever needed, avoiding the global
  • reorder scripts so that the app setup still works as expected

TODO:

  • CI passes
  • test deploy site and confirm no regressions

@shiftkey shiftkey merged commit 4264b45 into up-for-grabs:gh-pages Oct 5, 2019
@shiftkey shiftkey deleted the async-site-setup branch October 5, 2019 16:16
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.

1 participant