Introduce html oriented manifest format (introduces better Embroider interop) #272

thoov · 2020-05-12T23:00:30Z

This PR introduces a new htmlEntrypoint property on the manifest which allows this entry point to be the source of truth finding assets instead of them being defined as apart of the manifest itself. The primary use case for this is to allow embroider's asset chunking to more naturally work. This is a simplification from an earlier attempt (ember-fastboot/ember-cli-fastboot#753) which can be closed after this is merged.

This also introduces a concept of <fastboot-script src=""></fastboot-script> which is used to ignore scripts not intended for the browser. Here is how embroider uses these scripts:

embroider usage 1 / embroider usage 2

These changes are opt-in and backwards compatible.

cc: @rwjblue / @kratiahuja

This PR is co-authored by @ef4

kratiahuja · 2020-05-13T22:19:08Z

src/ember-app.js

+    if (manifest.htmlEntrypoint) {
+      let htmlEntrypoint = require('./html-entrypoint');
+      ({ appFiles, vendorFiles, html } = htmlEntrypoint(distPath, manifest.htmlEntrypoint));
+    } else {


High level question: With this new API would addons be able to use updateFastBootManifest hook + embroider?

If so, can we add a test for it?

I don't understand how that test would be relevant to this repo. fastboot only consumes the manifest, it doesn't have anything to do with constructing the manifest.

(Embroider itself has test coverage to ensure that existing addons that use updateFastbootManifest work correctly, starting from https://github.com/embroider-build/embroider/blob/b88f660cc6a6629de916123318c909d5efd3d8a9/test-packages/fastboot-addon/index.js#L6)

I understand that it only reads the config. I want to make sure the construct isn't broken. If it is in the embroider repo, I am fine with it.

kratiahuja · 2020-05-13T22:20:23Z

src/ember-app.js

-    let vendorFiles = manifest.vendorFiles.map(function(vendorFile) {
-      return path.join(distPath, vendorFile);
-    });
+    if (manifest.htmlEntrypoint) {


Is ember-cli-fastboot writing this into the manifest? or is it embroider?

Asking because we have a concept of FastBootSchemaVersions to ensure there is compatibility between the addon and this JS library.

It's written by embroider.

kratiahuja · 2020-05-13T22:24:58Z

test/fixtures/html-entrypoint/index.html

+    <fastboot-script src="assets/vendor.js"></fastboot-script>
+    <fastboot-script src="assets/fastboot-test.js"></fastboot-script>


How does embroider add the script meta tags in the final built index.html? I don't see it as part of test fixtures. Am I missing anything?

That is definitely beyond the scope of this repo, but if you are curious it happens in embroider/core here and here.

I'm not trying to be evasive here, I think it's important that we prioritize interface over implementation. If fastboot actually needed to care about where the script tags come from, then it would mean this is a bad interface because it leaks too much implementation.

Oh, I misunderstood your question. Sorry. I was thinking about <fastboot-script>, but you must have meant<meta name="your-app/config/environment">.

I will make sure I show that as I incorporate all the feedback into this PR.

rwjblue · 2020-05-13T22:17:05Z

package.json

@@ -30,6 +30,7 @@
    "debug": "^4.1.1",
    "resolve": "^1.15.0",
    "simple-dom": "^1.4.0",
+    "simple-html-tokenizer": "^0.5.9",


FWIW, I think this is probably blocked on landing tildeio/simple-html-tokenizer#71

I only picked simple-html-tokenizer because that's what the bikeshed landed on in the previous PR.

IMO it would be better to use a more fully-featured off-the-shelf HTML parser and avoid issues like this. It only runs once at server boot, it's not like it's on any performance critical path.

Seems fine, but we either need to land that PR or use a different parser. As it stands, we cannot parse properly formatted HTML.

rwjblue · 2020-05-13T22:34:58Z

src/ember-app.js

+    if (manifest.htmlEntrypoint) {
+      let htmlEntrypoint = require('./html-entrypoint');
+      ({ appFiles, vendorFiles, html } = htmlEntrypoint(distPath, manifest.htmlEntrypoint));
+    } else {
+      debug('reading array of app file paths from manifest');
+      appFiles = manifest.appFiles.map(function(appFile) {
+        return path.join(distPath, appFile);
+      });
+
+      debug('reading array of vendor file paths from manifest');
+      vendorFiles = manifest.vendorFiles.map(function(vendorFile) {
+        return path.join(distPath, vendorFile);
+      });
+
+      htmlFile = path.join(distPath, manifest.htmlFile);
+    }


Seems like this should actually be incrementing the schema version (see here) as the opt-in and we "upgrade" to the latest version automatically. That reduces overall branching in this code, and means that fastboot itself can always assume the same thing.

If you are ready to move all apps to this new format, then I agree and we can make this v5. I was not being so forward as to suggest that yet. At the moment it is an alternative, not an upgrade.

Making it an upgrade would certainly improve the error message people get if they try to use embroider without updating to this version of fastboot.

As for less branching: I realize that's the goal, but I don't actually agree that the current schema versioning system achieves it. Making this new format v5 would not result in fewer branches.

If you are ready to move all apps to this new format, then I agree and we can make this v5. I was not being so forward as to suggest that yet. At the moment it is an alternative, not an upgrade.

I see no reason to keep two long running branches here, there should be one logical successor with the other path being planned for deprecation/removal.

AFAICT, there really isn't a reason that ember-cli-fastboot can't emit htmlEntrypoint instead of its current system reducing yet another difference between Embroider and classic build worlds...

As for less branching: I realize that's the goal, but I don't actually agree that the current schema versioning system achieves it.

Ya, not sure I agree, but that isn't really the point. The goal of the system is to ensure we only deal with grabbing information from the manifest in a single place and we know what kind of thing we are dealing with. As it stands, you could easily have an htmlManifest and vendorFiles and have no clue why things don't work as you expect.

+1 if we think we have settled on htmlEntrypoint being the API, it would be ideal for ember-cli-fastboot to adopt it as well and reduce the clutter of maintaining between the two build systems.

it would be ideal for ember-cli-fastboot to adopt it as well and reduce the clutter of maintaining between the two build systems

Ya, agree. Once we land this, we should make a tracking issue over on ember-cli-fastboot.

rwjblue · 2020-05-13T22:35:53Z

src/html-entrypoint.js

+  for (let token of tokenize(html)) {
+    if (token.type === 'StartTag' && ['script', 'fastboot-script'].includes(token.tagName)) {
+      for (let [name, value] of token.attributes) {
+        if (name === 'src') {
+          appFiles.push(path.join(distPath, value));
+        }
+      }
+    }
+  }


FWIW, @stefanpenner wrote https://github.com/stefanpenner/find-scripts-srcs-in-document specifically for this use case.

Yes, but that library doesn't actually meet the requirements here (we need to control the set of tag names it matches), and we're talking about an 18 line function that is used by exactly zero other packages.

Reasonable people can disagree on the "lots of tiny libraries" approach. For me, this falls far, far below the bar of deserving to be its own library. It's hyper-specialized for this one use case. People doing anything more general will reach for a complete parser instead.

The library was only created so that we could add proper tests for various combinations (and really just started as a one off script that we wrote together inline with some tests to confirm things worked). Seems 100% fine to have this logic inlined, but we need to beef up the test coverage for the parsing bit.

bobisjan · 2020-05-13T22:44:58Z

src/html-entrypoint.js

+    if (token.type === 'StartTag' && ['script', 'fastboot-script'].includes(token.tagName)) {
+      for (let [name, value] of token.attributes) {
+        if (name === 'src') {
+          appFiles.push(path.join(distPath, value));


Based on the testing within our application this should skip external scripts like

<script src="https://cdn.onesignal.com/sdks/OneSignalSDK.js" defer=""></script>

which results in FastBoot error

Error: ENOENT: no such file or directory, open '/var/folders/4z/h7wb5mp50sxbqpt8bdt_nlg40000gn/T/broccoli-60630hW7MihRllKHG/out-666-packager_runner_embroider_webpack/https:/cdn.onesignal.com/sdks/OneSignalSDK.js'

That is a good point, we already have this policy in embroider itself when it consumes the same HTML. We should only extract relative URLs, and we should probably keep the earlier suggestion of a data-fastboot-ignore attribute.

This implements a new v5 schema for fastboot. Unlike the previous json-oriented formats, this is an html-oriented format. HTML is better understood by general purpose build tools, which makes it easier to integrate with embroider. It also allows us to eliminate some redundancies -- for example, the app config no longer needs to exist in both package.json and the meta tags, since the HTML oriented format can use it directly from the meta tags. In package.json, the v5 schema supports: - `fastboot.schemaVersion: 5` - `fastboot.moduleWhitelist: []`: unchanged meaning from v4. - `fastboot.htmlEntrypoint: 'index.html'`: the path to your HTML entrypoint file - `name`: note that in the new format, we take the appName directly from the package.json top level. This field is used for nothing else by the time we get to fastboot, so there was no reason for redundancy. The HTML entrypoint file is any valid HTML, plus these fastboot-specific extensions: - `<fastboot-script>` tags work like `<script>` tags, but they will only run in fastboot and be stripped out before serving to browsers. - `<script data-fastboot-ignore></script>` will *not* run in fastboot, and the data attribute will be stripped out before serving to browsers. - `<script src="https://some-cdn.com/app.js" data-fastboot-src="assets/app.js"></script>` tells fastboot the local path to an asset that may have a different public URL for browser consumption. The data-fastboot-src attribute is stripped before serving to browsers. New html-oriented manifest format for embroider implementing data-fastboot-ignore and data-fastboot-src making htmlEntrypoint the new v5 schema And refactoring so we truly hide the differences in schema versions from the rest of the program. schema v5 This makes the htmlEntrypoint format be the official v5 schema.

ef4 · 2020-05-19T17:13:25Z

I pushed a new (squashed) commit that incorporates the feedback to become the true v5 schema. This created a couple opportunities for simplification. Here is a summary of the complete new format:

In package.json, the v5 schema supports:

fastboot.schemaVersion: 5
fastboot.moduleWhitelist: []: unchanged meaning from v4.
fastboot.htmlEntrypoint: 'index.html': the path to your HTML entrypoint file
name: note that in the new format, we take the appName directly from the package.json top level. This field is used for nothing else by the time we get to fastboot, so there was no reason for redundancy.

All other fields that used to be supported in v4 under fastboot are gone. There is no manifest (the HTML is really our manifest now), and there is no config (the HTML already contains the config too, no need for redundancy).

The HTML entrypoint file is any valid HTML, plus these fastboot-specific extensions:

<fastboot-script> tags work like <script> tags, but they will only run in fastboot and be stripped out before serving to browsers.
<script data-fastboot-ignore></script> will not run in fastboot, and the data attribute will be stripped out before serving to browsers.
<script src="https://some-cdn.com/app.js" data-fastboot-src="assets/app.js"></script> tells fastboot the local path to an asset that may have a different public URL for browser consumption. The data-fastboot-src attribute is stripped before serving to browsers.

ef4 · 2020-05-19T20:16:26Z

We found one issue while testing this against some apps -- we need to account for the configured rootURL. I will add another commit that covers this case.

kratiahuja · 2020-05-20T22:30:45Z

tags work like <script> tags, but they will only run in fastboot and be stripped out before serving to browsers.

Curious to understand who is responsible for stripping out fastboot-script before serving to browsers? Do we expect web servers serving the index.html to do so?

rwjblue · 2020-05-21T01:13:45Z

@kratiahuja maybe the idea is that it just doesn't matter? It would show up as an HTML element (e.g. you could see it in the DOM explorer) but it would have no effect (wouldn't load the asset, wouldn't show any visible content). 🤔

ef4 · 2020-05-21T04:18:48Z

Curious to understand who is responsible for stripping out fastboot-script before serving to browsers?

fastboot is responsible, that feature is part of this PR. See https://github.com/ember-fastboot/fastboot/pull/272/files#diff-bc0f6350814e68802320bd056d350a09R28-R29

ef4 · 2020-05-21T04:19:38Z

Unless you mean, "when not using fastboot", in which case, yes, it's harmless if it stays in, but the assumption is that if you're using fastboot, your HTML is being served by fastboot.

kratiahuja · 2020-05-21T21:29:24Z

Unless you mean, "when not using fastboot", in which case, yes, it's harmless if it stays in, but the assumption is that if you're using fastboot, your HTML is being served by fastboot.

yes not in the case of not using fastboot. Example LinkedIn apps we will end up with those scripts.

kratiahuja

LGTM overall!

kratiahuja · 2020-05-22T19:39:17Z

src/fastboot-schema.js

+  if (schemaVersion < FastBootSchemaVersions.htmlEntrypoint) {
+    ({ appName, config, html, scripts } = loadManifest(distPath, pkg.fastboot, schemaVersion));
+  } else {
+    appName = pkg.name;
+    ({ config, html, scripts } = htmlEntrypoint(appName, distPath, pkg.fastboot.htmlEntrypoint));
+  }


Nitpick: It might be good to encapsulate this condition inside loadManifest itself. From where we read it shouldn't matter for loading config.

We no longer have a thing called "manifest" in the htmlEntrypoint case though, that's why I split it here.

kratiahuja · 2020-05-22T20:32:12Z

src/html-entrypoint.js

+    if (name && name.endsWith('/config/environment')) {
+      let content = JSON.parse(decodeURIComponent(element.getAttribute('content')));
+      content.APP = Object.assign({ autoboot: false }, content.APP);
+      config[name.slice(0, -1 * '/config/environment'.length)] = content;


Is there a way to avoid this? Seems a bit hacky IMO

Would you prefer name.split('/')[0]?

yes that is a lot clearer :) Thanks!

kratiahuja · 2020-05-22T21:04:41Z

@ef4 This lgtm overall, feel free to merge it (or let me know if you would like me to) .

thoov · 2020-05-26T05:21:38Z

@kratiahuja yes could you merge and release?

rwjblue · 2020-05-26T13:48:35Z

https://github.com/ember-fastboot/fastboot/releases/tag/v3.1.0

xg-wang · 2021-04-06T16:31:45Z

@ef4 / @thoov does the new html oriented manifest support https://github.com/ember-fastboot/ember-cli-fastboot/tree/e4d0b7c7bcdf82def0dc8726835b49d707673f41#providing-additional-config? I can't find where embroider writes fastboot specific config into the meta tag

ef4 · 2021-04-09T15:59:59Z

I think the fastboot-specific config still lives directly in the fastboot manifest, but I haven't looked at this lately.

kratiahuja reviewed May 13, 2020

View reviewed changes

rwjblue reviewed May 13, 2020

View reviewed changes

bobisjan reviewed May 13, 2020

View reviewed changes

ef4 force-pushed the html-entrypoint branch from 8f020cc to 3fd5bc9 Compare May 19, 2020 17:09

handle customized rootURL

3fae95c

thoov requested review from rwjblue and kratiahuja May 21, 2020 21:21

kratiahuja reviewed May 22, 2020

View reviewed changes

rwjblue approved these changes May 26, 2020

View reviewed changes

rwjblue added the enhancement label May 26, 2020

rwjblue changed the title ~~New html-oriented manifest format for embroider~~ Introduce html oriented manifest format (introduces better Embroider interop) May 26, 2020

rwjblue merged commit 429eb1d into ember-fastboot:master May 26, 2020

		<fastboot-script src="assets/vendor.js"></fastboot-script>
		<fastboot-script src="assets/fastboot-test.js"></fastboot-script>

Introduce html oriented manifest format (introduces better Embroider interop) #272

Introduce html oriented manifest format (introduces better Embroider interop) #272

Conversation

thoov commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ef4 commented May 19, 2020

ef4 commented May 19, 2020

kratiahuja commented May 20, 2020

rwjblue commented May 21, 2020

ef4 commented May 21, 2020

ef4 commented May 21, 2020

kratiahuja commented May 21, 2020

kratiahuja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratiahuja commented May 22, 2020

thoov commented May 26, 2020

rwjblue commented May 26, 2020

xg-wang commented Apr 6, 2021

ef4 commented Apr 9, 2021

rwjblue May 14, 2020 •

edited

Loading