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

Boilerplate refactor #8820

Merged
merged 26 commits into from Jul 10, 2017

Conversation

Projects
None yet
6 participants
@stevenhao
Contributor

stevenhao commented Jun 19, 2017

Summary of changes

This PR refactors the boilerplate generator to generate the html string using javascript instead of using Blaze with Spacebars templates. Except for some minor differences (whitespace, html-escaping), the generated html is the same as it was before the refactor.

Changes to the boilerplate-generator package:

  • Bump version to 1.1.2
  • Rename and modernize boilerplate-generator.js to generator.js
  • Delete boilerplate_web.browser.html and boilerplate_web.cordova.html
  • Add template_web.browser.js and template_web.cordova.js

This PR also adds a boilerplate-generator-tests package (previously, all the boilerplate-related tests were tested indirectly in the webapp package).

@stevenhao stevenhao requested a review from glasser Jun 19, 2017

@meteor-bot

This comment has been minimized.

meteor-bot commented Jun 19, 2017

@stevenhao: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -11,11 +11,9 @@ Boilerplate = function (arch, manifest, options) {
options = options || {};

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

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?

This comment has been minimized.

@benjamn

benjamn Jun 20, 2017

Member

Let's modernize this code! It runs on the server anyway. Specifically, @stevenhao, we should put api.use("ecmascript", "server") in package.js, define an api.mainModule, and use class syntax (etc.) in this file.

return "<!DOCTYPE html>\n" +
Blaze.toHTML(Blaze.With(_.extend(self.baseData, extraData),
self.func));
return "<!DOCTYPE html>\n" + self.template(_.extend(self.baseData, extraData));

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

_.extend modifies self.baseData in place — is that intentional? I think maybe _.extend({}, self.baseData, extraData) may be better. I see that's in the existing code too but maybe that's a bug.

This comment has been minimized.

@benjamn

benjamn Jun 20, 2017

Member

Here's an idea you can ignore for the time being:

I would love to enable more interesting extensions of self.baseData here, specifically to support server-side rendering. For example, if this code called some custom callback with self.baseData, then the callback could parse the self.baseData.body HTML and attach server-rendered fragments to DOM nodes with particular ids. Then the client could use document.getElementById to find the same nodes and rehydrate the server-generated HTML.

For now, let's focus on feature parity, but I wanted to plant that seed. 🌱

This comment has been minimized.

@stevenhao

stevenhao Jun 21, 2017

Contributor

yup, i'll change it to _.extend({}, self.baseData, extraData), or if it's safe to use ES6, Object.assign({}, self.baseData, extraData)

This comment has been minimized.

@glasser

glasser Jun 22, 2017

Member

ES6 is definitely fine in server code and I think in client code too.

// 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) {

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

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.

// XXX is htmlAttributes ever anything but {}?
// may just be a generic Blaze/Spacebars thing.
function props1(htmlAttributes) {

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

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 "".

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

Definitely want to have some escaping here! Not just wrapping, also doing proper encoding

This comment has been minimized.

@stevenhao

stevenhao Jun 21, 2017

Contributor

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 _.template(' <%= key %>="<%- value %>"')({key, value}).

Potential issues:

  • The attribute namespace isn't explicitly handled and must be directly attached to the key of htmlAttributes, e.g. htmlAttributes may need to have entries like {"xmlns:h": "http://www.w3.org/2000/xmlns/"}.
  • _.template("<%- ... %>") does HTML escaping, does not distinguish between attribute mode. this project has an isAttributeValue option that may be useful. It seems to me that the attribute mode distinction isn't hugely important, and that most browsers handle unnecessarily escaped characters gracefully.

This comment has been minimized.

@glasser

glasser Jun 22, 2017

Member

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.

''
],
function if1(inlineScriptsAllowed) {

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

this one might be easier just as a ?: thing?

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) + '">';

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

url needs escaping (anything in the original template that's {{}} rather than {{{}}} should be escaped). Note that escaping is either URL-escaping or HTML-escaping depending on where it is, I think?

Package.onUse(function (api) {
api.use([
'underscore',

This comment has been minimized.

@glasser

glasser Jun 19, 2017

Member

Given that we're relying on underscore anyway, maybe just use _.template with a template stored in a file and read by Assets.getText?

This comment has been minimized.

@stevenhao

stevenhao Jun 19, 2017

Contributor

I had no idea this existed! I'll try it out.

@benjamn benjamn added this to the Release 1.5.1 milestone Jun 21, 2017

@abernix abernix changed the title from [WIP] [Do not Merge] Boilerplate refactor to [Work in Progress] Boilerplate refactor Jun 21, 2017

var fs = Npm.require('fs');
var path = Npm.require('path');
import fs from 'fs';
import path from 'path';

This comment has been minimized.

@benjamn

benjamn Jun 22, 2017

Member

I would be more specific:

import { readFile } from "fs";
import { ??? } from "path";

It looks like you're not using path anywhere in this file, so I think you can remove that line.

);
};
export class Boilerplate {
constructor (arch, manifest, options = {}) {

This comment has been minimized.

@benjamn

benjamn Jun 22, 2017

Member

We don't generally put a space after method names (before the ().

Blaze.toHTML(Blaze.With(_.extend(self.baseData, extraData),
self.func));
};
return "<!DOCTYPE html>\n" + this.template(Object.assign({}, this.baseData, extraData));

This comment has been minimized.

@benjamn

benjamn Jun 22, 2017

Member

This is pretty long line that might be worth breaking after the +, as before.

// Replicates the template defined in boilerplate_web.cordova.html
// Arguments: root : { htmlAttributes, css : [{ url }], bundledJsCssUrlRewriteHook : Function, head, dynamicHead, body, dynamicBody, inlineScriptsAllowed, additionalStaticJs, meteorRuntimeConfig }
export default function(manifest) {

This comment has been minimized.

@benjamn

benjamn Jun 22, 2017

Member

We do generally put a space after the function keyword (before the () for anonymous functions.

// XXX the template we are evaluating relies on the fact that UI is globally
// available.
global.UI = UI;
self.func = eval(boilerplateRenderCode);

This comment has been minimized.

@benjamn

benjamn Jun 22, 2017

Member

Glad to see this eval go away!

'</html>'
],
['', '<!-- Generated for cordova by boilerplate-generator -->']

This comment has been minimized.

@benjamn

benjamn Jun 22, 2017

Member

Should this be referring to Cordova, if this is boilerplate_web_browser_template.js?

return Assets.getText(filename);
// Returns a template function that, when called, produces the boilerplate
// html as a string.
const _getTemplate = _.memoize(arch => {

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

I don't think the memoize is necessary anymore now that the function isn't doing disk IO.

// Arguments: root : { htmlAttributes, css : [{ url }], bundledJsCssUrlRewriteHook : Function, head, dynamicHead, body, dynamicBody, inlineScriptsAllowed, additionalStaticJs, meteorRuntimeConfig }
export default function (manifest) {
const root = manifest;

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

Not sure why you're renaming the parameter instead of just giving it whatever name you want it to have.

[
'<html' +_.map(root.htmlAttributes, (value, key) =>
_.template(' <%= attrName %>="<%- attrValue %>"')({
attrName: key,

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

A little confusing to choose to call it "key" and then a line later call the same thing "name"

This comment has been minimized.

@stevenhao

stevenhao Jun 26, 2017

Contributor

I agree that it's a bit confusing, but I was hoping that it would make sense:

  • (value, key) correspond to {key: value} pairs
  • attrName and attrValue are are templated as name="value" HTML attributes.

I'm happy to change it to be '<% key %>="<%- value %>"' if you think that's less confusing.

? ' <script type="text/javascript"><%= contents %></script>'
: ' <script type="text/javascript" src="<%- src %>"></script>'
)({
contents: _.template('__meteor_runtime_config__ = JSON.parse(decodeURIComponent(<%= conf %>))')({

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

Not sure why you're doing nested calls to _.template instead of just putting this in the first template. Also, it's kinda weird that you're providing both keys (contents/src) to either template... why not do

root.inlineScriptsAllowed ?
  _.template('template1', args1) : _.template('template2', args2)

?

Also in the old version, meteorRuntimeConfig was {{}} (HTML-escaped) though it probably doesn't matter due to the use of encodeURIComponent.

contents: _.template('__meteor_runtime_config__ = JSON.parse(decodeURIComponent(<%= conf %>))')({
conf: root.meteorRuntimeConfig
}),
src: root.rootUrlPathPrefix + '/meteor_runtime_config.js'

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

the second half here can just go in the template...

),
_.map(root.additionalStaticJs, ({contents, pathname}) => (
_.template(root.inlineScriptsAllowed

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

similarly, this should probably be separate template calls

_.map(root.additionalStaticJs, ({contents, pathname}) => (
_.template(root.inlineScriptsAllowed
? ' <script><%= contents %></script>'

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

Looks like contents should actually be escaped.

This comment has been minimized.

@stevenhao

stevenhao Jun 26, 2017

Contributor

It seems like we have to be careful with over-escaping contents here, because some browsers don't handle escaping very well when it's within a script tag.

This Gist shows how unexpected behavior can occur, even on master.

This comment has been minimized.

@stevenhao

stevenhao Jun 27, 2017

Contributor

Discussed this with @benjamn, we decided that it's probably safest to load all additionalStaticJs from the server. The reason is that embedding arbitrary javascript inline is tricky to do correctly, and can cause strange bugs.

The tradeoff we're making here is that loading from the server may be more costly than inlining the script. The only internal package that is loaded through additionalStaticJs is https://github.com/meteor/meteor/tree/master/packages/reload-safetybelt, and as far as I'm aware, external packages don't really use WebAppInternals.addStaticJs. Correct me if I'm wrong, but it seems like making this change won't be too disruptive?

This comment has been minimized.

@glasser

glasser Jun 27, 2017

Member

Hmm, I mean, this was an explicit choice to minimize round trips (ie, startup time) for apps, and I think it's been working for many years? If it's only used by reload-safetybelt, a weird package we don't really even recommend, then I guess it's not a big deal, but in that case maybe it's not a big deal in either direction (if we are confident that reload-safetybelt's code is escaped correctly)?

),
_.map(root.additionalStaticJs, ({pathname, contents}) =>
_.template(inlineScriptsAllowed
? ' <script type="text/javascript"><%= contents %></script>'

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

escape, use multiple templates.

Although honestly I'm not sure why we check for inlineScriptsAllowed here when there's already a giant inline script above. Seems like a bug. Not sure who actually knows about this stuff anymore — overlap of the browser content policy package and Cordova. @benjamn ? @martijnwalraven ?

This comment has been minimized.

@stevenhao

stevenhao Jun 27, 2017

Contributor

I'm going to err on the side of caution and not change the behavior of this section. If we decide that we'll be inlining all scripts for cordova, or inline no scripts for browser (or both), I think that can be a separate PR.

'</html>'
],
['', '<!-- Generated for cordova by boilerplate-generator -->']

This comment has been minimized.

@glasser

glasser Jun 24, 2017

Member

Are these comments going to stick around forever?

This comment has been minimized.

@stevenhao

stevenhao Jun 27, 2017

Contributor

I thought it might be nice to leave a note in generated code about how it was generated, but maybe it doesn't make sense to expose this to the public

benjamn added a commit that referenced this pull request Jun 26, 2017

Don't modify boilerplate.baseData when adding extraData.
This is a bug that will be fixed by @stevenhao's boilerplate-generator
refactoring (#8820), but I need it fixed now :)

benjamn added a commit that referenced this pull request Jun 26, 2017

Don't modify boilerplate.baseData when adding extraData.
This is a bug that will be fixed by @stevenhao's boilerplate-generator
refactoring (#8820), but I need it fixed now :)

@mitar mitar referenced this pull request Jun 27, 2017

Closed

SRI support #8847

@stevenhao stevenhao changed the title from [Work in Progress] Boilerplate refactor to Boilerplate refactor Jun 27, 2017

@glasser

This comment has been minimized.

Member

glasser commented Jun 27, 2017

Looking generally good. I'll leave what to do about additionalStaticJs up to Meteor team. This is the last feedback I can give until back from vacation next Wednesday — I'm sure Meteor team can help you finish this one up :)

benjamn added a commit that referenced this pull request Jun 30, 2017

Don't modify boilerplate.baseData when adding extraData.
This is a bug that will be fixed by @stevenhao's boilerplate-generator
refactoring (#8820), but I need it fixed now :)

stevenhao added some commits Jun 30, 2017

Revert "dummy commit"
This reverts commit 54fe867.

@stevenhao stevenhao force-pushed the stevenhao:boilerplate-refactor-2 branch from 54a8ad1 to 59f85ed Jul 5, 2017

@abernix

Awesome! I think functionally this looks great (and works!), but I've added my comments below, some of which are worth addressing. Mostly nitpicks and formatting though!

url: '/packages/bootstrap/css/bootstrap-responsive.css?hash=785760fc5ad665d7b54d56a3c2522797bb2cc150&v="1"',
size: 22111,
hash: '785760fc5ad665d7b54d56a3c2522797bb2cc150' },
{ path: 'packages/templating-runtime.js',

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

The indentation of this entry in the array should be the same as the one above it (one more space to the right).

cacheable: true,
url: '/packages/bootstrap/css/bootstrap-responsive.css?hash=785760fc5ad665d7b54d56a3c2522797bb2cc150&v="1"',
size: 22111,
hash: '785760fc5ad665d7b54d56a3c2522797bb2cc150' },

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

Let's align this closing brace with the opening brace.

cacheable: true,
url: '/packages/templating-runtime.js?hash=c18de19afda6e9f0db7faf3d4382a4c953cabe18&v="1"',
size: 24132,
hash: 'c18de19afda6e9f0db7faf3d4382a4c953cabe18' },

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

This brace should also be on its own line.

_.map(js, ({url}) =>
_.template(' <script type="text/javascript" src="<%- src %>"></script>')({
src: bundledJsCssUrlRewriteHook(url) + '&hello=bye'

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

This hello=bye seems out of place. Is it meant to accomplish something?

This comment has been minimized.

@stevenhao

stevenhao Jul 6, 2017

Contributor

ah, forgot to remove this from testing! thanks for catching it

@@ -0,0 +1,90 @@
import { parse, serialize } from 'parse5';
function generateHTML() {

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

With the exception of the arch variable on the next line, it seems this is all duplicatd from the test in browser_test.js. We could DRY this up by making the generateHTML an import from a supporting library (for example, test-lib.js) that lives alongside the cordova_test.js and browser_test.js files, and supplying it with arch as a parameter.

// instead it should connect to 10.0.2.2
// (unless we\'re using an http proxy; then it works!)
' if (!__meteor_runtime_config__.httpProxyPort) {',
' __meteor_runtime_config__.ROOT_URL = (__meteor_runtime_config__.ROOT_URL || \'\').replace(/localhost/i, \'10.0.2.2\');',

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

I know some linting algorithms or style-guides may unnecessarily chastise this, but on the Meteor project, feel free to switch to double quotes to avoid the excessive escaping of single-quotes. (i.e. to avoid having to \'\')

' if (/Android/i.test(navigator.userAgent)) {',
// When Android app is emulated, it cannot connect to localhost,
// instead it should connect to 10.0.2.2
// (unless we\'re using an http proxy; then it works!)

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

We don't need to escape the ' in this comment (it's not part of a string), but we should think about whether this comment was providing any value in the original template since it's no longer included now. (I think the value is low, so mainly just pointing this out).

Tinytest.add("boilerplate-generator-tests - web.cordova escape css", function (test) {
const html = generateHTML();
test.matches(html, /<link.*href=".*bootstrap.*&amp;v=&quot;1&quot;.*">/);

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

Not that it would happen, but just pointing out that this will also match:

<link rel="stylesheet" type="text/css" class="__meteor-css__" href="&v="1"bootstrap&amp;v=&quot;1&quot;">

Is that what you wanted? Maybe better to test for the absence of quotes and & or for an exact attribute value match? This might be easy to accomplish using parse5's parse?

test.matches(html, /<link.*href=".*bootstrap.*&amp;v=&quot;1&quot;.*">/);
});
Tinytest.add("boilerplate-generator-tests - web.cordova do not call rewriteHook", function (test) {

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

Just informational, but if you add another - after web.cordova in these test names, they will be grouped into their own section in tinytest reports. :)

@@ -0,0 +1,100 @@
import { parse, serialize } from 'parse5';

This comment has been minimized.

@abernix

abernix Jul 6, 2017

Member

I think the name of this file should be consistent with that of the appropriate arch template (i.e. boilerplate_web.browser.template.js), and it should be plural like our other test files (e.g. web.browser-tests.js, web.cordova-tests.js)

abernix added a commit that referenced this pull request Jul 7, 2017

Remove spaces after object-shorthand method names.
Per the comment in
  #8820 (comment)

Previously, only the `constructor` method was addressed and this expands
on that.

abernix added some commits Jul 7, 2017

Remove spaces after object-shorthand method names.
Per the comment in
  #8820 (comment)

Previously, only the `constructor` method was addressed and this expands
on that.
@hwillson

This comment has been minimized.

Member

hwillson commented Jul 7, 2017

This is looking great @stevenhao! One small thing to mention (and it's not necessarily something that should be considered with these changes, but I'm just mentioning it for discussion). With these changes we're still forcing the Meteor generated CSS to come before any externally referenced CDN based CSS files (in <head />). For the full backstory, see meteor/meteor-feature-requests#24 (and #8651). It's quite common practice to have 3rd party CDN based externally referenced CSS libraries come first in the CSS load order, which are then overridden as necessary by an application's CSS. Since we're refactoring the boilerplate it might be nice to also consider handling this scenario. Anyways, just food for thought - thanks for all of your work on this!

@stevenhao

This comment has been minimized.

Contributor

stevenhao commented Jul 7, 2017

@hwillson, thanks for the comments! I agree that forcing the bundled css to come before custom css is not ideal, but I'm worried about backwards compatibility. Adding a configurable
flag to reorder stylesheets is a possible solution, but that may be out of the scope of this PR. What do you think @abernix?

@glasser glasser dismissed their stale review Jul 7, 2017

Letting @abernix take it from here

@abernix abernix modified the milestones: Release 1.5.2, Release 1.5.1 Jul 10, 2017

@abernix abernix merged commit 98b02f3 into meteor:devel Jul 10, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abernix

This comment has been minimized.

Member

abernix commented Jul 10, 2017

It's certainly worth mentioning the CSS load order here, but I think we should wrap this PR up and then iterate on it again as necessary.

This appears to be working properly in that when using static-html the server-side dependency on blaze is removed which was a goal of this PR (client was already removable). It's worth pointing out that there is still a dependency on templating-tools (in the Blaze repo) because of:

  1. The CompileError class (via throwCompileError, here)
  2. TemplatingTools.scanHtmlForTags.

I made a couple formatting changes but this is great work, @stevenhao!

GulajavaMinistudio added a commit to GulajavaMinistudio/meteor that referenced this pull request Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment