Boilerplate refactor #8820
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| var fs = Npm.require('fs'); | ||
| var path = Npm.require('path'); | ||
|
|
||
| // Copied from webapp_server | ||
| var readUtf8FileSync = function (filename) { | ||
| return Meteor.wrapAsync(fs.readFile)(filename, 'utf8'); | ||
| }; | ||
|
|
||
| Boilerplate = function (arch, manifest, options) { | ||
| var self = this; | ||
| options = options || {}; | ||
| self.template = _getTemplate(arch); | ||
| self.baseData = null; | ||
| self.func = null; | ||
|
|
||
| self._generateBoilerplateFromManifestAndSource( | ||
| manifest, | ||
| self.template, | ||
| options | ||
| ); | ||
| }; | ||
|
|
||
| // The 'extraData' argument can be used to extend 'self.baseData'. Its | ||
| // purpose is to allow you to specify data that you might not know at | ||
| // the time that you construct the Boilerplate object. (e.g. it is used | ||
| // by 'webapp' to specify data that is only known at request-time). | ||
| Boilerplate.prototype.toHTML = function (extraData) { | ||
| var self = this; | ||
|
|
||
| if (! self.baseData || ! self.func) | ||
| throw new Error('Boilerplate did not instantiate correctly.'); | ||
|
|
||
| return "<!DOCTYPE html>\n" + | ||
| Blaze.toHTML(Blaze.With(_.extend(self.baseData, extraData), | ||
| self.func)); | ||
| }; | ||
|
|
||
| // XXX Exported to allow client-side only changes to rebuild the boilerplate | ||
| // without requiring a full server restart. | ||
| // Produces an HTML string with given manifest and boilerplateSource. | ||
| // Optionally takes urlMapper in case urls from manifest need to be prefixed | ||
| // or rewritten. | ||
| // Optionally takes pathMapper for resolving relative file system paths. | ||
| // Optionally allows to override fields of the data context. | ||
| Boilerplate.prototype._generateBoilerplateFromManifestAndSource = | ||
| function (manifest, boilerplateSource, options) { | ||
| var self = this; | ||
| // map to the identity by default | ||
| var urlMapper = options.urlMapper || _.identity; | ||
| var pathMapper = options.pathMapper || _.identity; | ||
|
|
||
| var boilerplateBaseData = { | ||
| css: [], | ||
| js: [], | ||
| head: '', | ||
| body: '', | ||
| meteorManifest: JSON.stringify(manifest) | ||
| }; | ||
|
|
||
| // allow the caller to extend the default base data | ||
| _.extend(boilerplateBaseData, options.baseDataExtension); | ||
|
|
||
| _.each(manifest, function (item) { | ||
| var urlPath = urlMapper(item.url); | ||
| var itemObj = { url: urlPath }; | ||
|
|
||
| if (options.inline) { | ||
| itemObj.scriptContent = readUtf8FileSync( | ||
| pathMapper(item.path)); | ||
| itemObj.inline = true; | ||
| } | ||
|
|
||
| if (item.type === 'css' && item.where === 'client') { | ||
| boilerplateBaseData.css.push(itemObj); | ||
| } | ||
| if (item.type === 'js' && item.where === 'client' && | ||
| // Dynamic JS modules should not be loaded eagerly in the | ||
| // initial HTML of the app. | ||
| ! item.path.startsWith('dynamic/')) { | ||
| boilerplateBaseData.js.push(itemObj); | ||
| } | ||
| if (item.type === 'head') { | ||
| boilerplateBaseData.head = | ||
| readUtf8FileSync(pathMapper(item.path)); | ||
| } | ||
| if (item.type === 'body') { | ||
| boilerplateBaseData.body = | ||
| readUtf8FileSync(pathMapper(item.path)); | ||
| } | ||
| }); | ||
| var boilerplateRenderCode = SpacebarsCompiler.compile( | ||
| boilerplateSource, { isBody: true }); | ||
|
|
||
| // Note that we are actually depending on eval's local environment capture | ||
| // so that UI and HTML are visible to the eval'd code. | ||
| // XXX the template we are evaluating relies on the fact that UI is globally | ||
| // available. | ||
| global.UI = UI; | ||
| self.func = eval(boilerplateRenderCode); | ||
| self.baseData = boilerplateBaseData; | ||
| }; | ||
|
|
||
| var _getTemplate = _.memoize(function (arch) { | ||
| var filename = 'boilerplate_' + arch + '.html'; | ||
| return Assets.getText(filename); | ||
| }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| // Template function for rendering the boilerplate html | ||
| // Replicates the template defined in boilerplate_web.browser.html | ||
| // XXX Does not necessarily preserve formatting (e.g. additionalStaticJs newlines) | ||
| // Arguments: root : { htmlAttributes, css : [{ url }], bundledJsCssUrlRewriteHook : Function, head, dynamicHead, body, dynamicBody, inlineScriptsAllowed, additionalStaticJs, meteorRuntimeConfig } | ||
|
|
||
| Boilerplate_Web_Browser_Template = function(root) { | ||
|
There was a problem hiding this comment. This isn't our normal variable naming style. I think BoilerplateWebBrowserTemplate, or even just WebBrowserTemplate since we're not exporting it out of the package. |
||
| return [].concat( | ||
|
|
||
| // XXX is htmlAttributes ever anything but {}? | ||
| // may just be a generic Blaze/Spacebars thing. | ||
| function props1(htmlAttributes) { | ||
|
There was a problem hiding this comment. I don't think you need to name these functions that are only called immediately. But I also am unsure why this is necessary — can't you just write root.htmlAttributes directly? Is this based on starting with some generated code? I'd simplify this to look more like functions you'd write yourself. |
||
| return [ | ||
| ['<html'].concat(Object.keys(htmlAttributes).map(function(key) { | ||
|
|
||
| // XXX probably need to wrap strings in "". | ||
|
There was a problem hiding this comment. Definitely want to have some escaping here! Not just wrapping, also doing proper encoding There was a problem hiding this comment. Yeah, I've been delaying implementing this because I'm not sure what the best library for this kind of stuff. From what I can tell after reading https://github.com/meteor/blaze/blob/master/packages/htmljs/visitors.js#L198, https://html.spec.whatwg.org/multipage/syntax.html#attributes-2, an attribute pair should stringify to Potential issues:
There was a problem hiding this comment. we chatted about this in person and decided that we need to HTML-encode attribute values but not names, and don't need to URL-encode anything. |
||
| return key + '=' + htmlAttributes[key]; | ||
| })).join(' ') + '>' | ||
| ]; | ||
| }(root.htmlAttributes), | ||
|
|
||
| [ | ||
| '<head>' | ||
| ], | ||
|
|
||
| function each1(css) { | ||
| var bundledJsCssUrlRewriteHook = root.bundledJsCssUrlRewriteHook; | ||
| return _.map(css, function(obj) { | ||
| var url = obj.url; | ||
| return ' <link rel="stylesheet" type="text/css" class="__meteor-css__" href="' + bundledJsCssUrlRewriteHook(url) + '">'; | ||
|
There was a problem hiding this comment. url needs escaping (anything in the original template that's |
||
| }); | ||
| }(root.css), | ||
|
|
||
| [ | ||
| root.head, | ||
| root.dynamicHead, | ||
| '</head>', | ||
| '<body>', | ||
| root.body, | ||
| root.dynamicBody, | ||
| '' | ||
| ], | ||
|
|
||
| function if1(inlineScriptsAllowed) { | ||
|
There was a problem hiding this comment. this one might be easier just as a ?: thing? |
||
| var meteorRuntimeConfig = root.meteorRuntimeConfig; | ||
| var rootUrlPathPrefix = root.rootUrlPathPrefix; | ||
|
|
||
| if (inlineScriptsAllowed) { | ||
| return [ | ||
| '<script type="text/javascript">__meteor_runtime_config__ = JSON.parse(decodeURIComponent(' + meteorRuntimeConfig + '));</script>' // XXX add 2 spaces to fix indentation | ||
| ]; | ||
| } else { | ||
| return [ | ||
| '<script type="text/javascript" src="' + rootUrlPathPrefix + '/meteor_runtime_config.js"></script>' // XXX add 2 spaces to fix indentation | ||
| ]; | ||
| } | ||
| }(root.inlineScriptsAllowed), | ||
|
|
||
| [''], | ||
|
|
||
| function each2(js) { | ||
| var bundledJsCssUrlRewriteHook = root.bundledJsCssUrlRewriteHook; | ||
| return _.map(js, function(obj) { | ||
| var url = obj.url; | ||
| return ' <script type="text/javascript" src="' + bundledJsCssUrlRewriteHook(url) + '"></script>'; | ||
| }); | ||
| }(root.js), | ||
|
|
||
| function each3(additionalStaticJs) { | ||
| var inlineScriptsAllowed = root.inlineScriptsAllowed; | ||
| var rootUrlPathPrefix = root.rootUrlPathPrefix; | ||
| return _.map(additionalStaticJs, function(obj) { | ||
| var contents = obj.contents; | ||
| var pathname = obj.pathname; | ||
| if (inlineScriptsAllowed) { | ||
| return ' <script>' + contents + '</script>'; | ||
| } else { | ||
| return " <script type='text/javascript' src='" + rootUrlPathPrefix + pathname + "'></script>"; | ||
| } | ||
| }); | ||
| }(root.additionalStaticJs), | ||
|
|
||
| [ | ||
| '', '', | ||
| '</body>', | ||
| '</html>' | ||
| ], | ||
|
|
||
| ['', '<!-- Generated by boilerplate generator -->'] // to help distinguishes old generator from new [just for testing] | ||
| ).join('\n'); // undefined is treated as empty string (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/join) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,9 @@ Package.describe({ | |
| version: '1.1.0' | ||
| }); | ||
|
|
||
| var USE_OLD_BOILERPLATE_GENERATOR = true; | ||
| USE_OLD_BOILERPLATE_GENERATOR = false; // uncomment this line to use the new boilerplate generator | ||
| if(USE_OLD_BOILERPLATE_GENERATOR){ | ||
| Package.onUse(function (api) { | ||
| api.use([ | ||
| 'underscore', | ||
|
|
@@ -11,7 +14,7 @@ Package.onUse(function (api) { | |
| 'htmljs', | ||
| 'ui', | ||
| ], 'server'); | ||
| api.addFiles(['boilerplate-generator-old.js'], 'server'); | ||
| api.export(['Boilerplate'], 'server'); | ||
| // These are spacebars templates, but we process them manually with the | ||
| // spacebars compiler rather than letting the 'templating' package (which | ||
|
|
@@ -22,3 +25,16 @@ Package.onUse(function (api) { | |
| 'boilerplate_web.cordova.html' | ||
| ], 'server'); | ||
| }); | ||
| } else { | ||
|
|
||
| Package.onUse(function (api) { | ||
| api.use([ | ||
| 'underscore', | ||
|
There was a problem hiding this comment. Given that we're relying on underscore anyway, maybe just use There was a problem hiding this comment. I had no idea this existed! I'll try it out. |
||
| ], 'server'); | ||
| api.addFiles([ | ||
| 'boilerplate_web_template.js', | ||
| 'boilerplate-generator.js' | ||
| ], 'server'); | ||
| api.export(['Boilerplate'], 'server'); | ||
| }); | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really more of a question for @benjamn or @abernix but should new code be written with ES6 these days? import instead of Npm.require (it's ok even in packages to use import for built in packages like this one, right?), no var, class, etc? Using mainModule and imports instead of file lists and package-local variables?
And specifically should this refactored class be rewritten to new style or left alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's modernize this code! It runs on the server anyway. Specifically, @stevenhao, we should put
api.use("ecmascript", "server")inpackage.js, define anapi.mainModule, and useclasssyntax (etc.) in this file.