Added support to update manifest appfiles from index.html #701

dnalagatla · 2019-06-05T15:20:02Z

In order to support Embroider buld:

Added postBuild hook to update assetFiles property in the fastboot manifest to load in the sandbox.

* Also, as autoRun is false in fastboot and app-boot type not supported in contentFor hook. Moved the logic to postBuild. Used postBuild hook for appending fastboot module and a check to create app when not in fastboot environment. - Merged as part of this pr - #702

#700 - embroider-build/embroider#184

Tested Application using embroider build and application not using embroider build.

dnalagatla · 2019-06-05T15:27:45Z

@rwjblue @stefanpenner

index.js

tomdale · 2019-06-05T20:03:25Z

Cheerio is a pretty large dependency for FastBoot to take on.

Want to get feedback from @stefanpenner @ef4 @rwjblue that the correct architecture is for us to parse the HTML file (vs. emitting some kind of JSON manifest), and that cheerio is the simplest way to do that.

rwjblue · 2019-06-05T20:14:10Z

We should be able to parse with simple-html-tokenizer, which is already in our dep graph...

ef4 · 2019-06-05T20:28:57Z

Yes, parsing the HTML file is the way to go. It aligns fastboot with web standards instead of our own ember-only standard. It means that the output from Parcel or Webpack can go directly to fastboot and fastboot will be able to know how to boot it.

(The other piece of this story is that any browser-only or fastboot-only code will be guarded by the macro system. In development we can do a single build that keeps both branches in, while running correctly in both environments. In prod we can produce both optimized builds.)

dnalagatla · 2019-06-06T00:40:57Z

Cheerio is a pretty large dependency for FastBoot to take on.

Want to get feedback from @stefanpenner @ef4 @rwjblue that the correct architecture is for us to parse the HTML file (vs. emitting some kind of JSON manifest), and that cheerio is the simplest way to do that.

I am open to using any other library here for parsing the HTML. As this a build time parsing and cheerio being the dev dependency I don't see this causing issue during runtime.

One drawback of parsing HTML would be related to production builds replacing asset URLs with hashed CDN URLs. In that case, this postBuild processing would fail as hashed assets wouldn't be present in app's package on the disk. Any thoughts about this scenario @tomdale @stefanpenner @ef4 @rwjblue

rwjblue · 2019-06-06T14:23:28Z

One drawback of parsing HTML would be related to production builds replacing asset URLs with hashed CDN URLs.

@ef4 - Have you considered this case? Is this something that we would have to track and reverse the work of the fingerprinter?

ef4 · 2019-06-06T14:37:15Z

CDN prefixing and fingerprinting are two different concerns.

Fingerprinting does not need to be reversed (and can't be, in the case of embroider, because it's up to the stage3 packager to do whatever it wants with filenames).

The way we do CDN prefixing today, yes, it would need to be reversed. But that is much easier (it only requires one argument to fastboot, the prefix it's allowed to strip off URLs to make them local again). Arguably we don't even need that, because fastboot can see all the assets on disk and can correlate URL suffixes with filenames, allowing it to discover any CDN prefixes on the fly.

rwjblue · 2019-06-14T16:33:07Z

Now that #702 is landed, can you rebase?

dnalagatla · 2019-06-15T16:21:12Z

Now that #702 is landed, can you rebase?

Thank you @rwjblue I did the rebase. Updated the decription and title of this PR.

rwjblue · 2019-06-17T15:29:20Z

index.js

@@ -327,8 +328,7 @@ module.exports = {
  },

  postBuild(result) {
-    let appFilePath = stripLeadingSlash(this.app.options.outputPaths.app.js);
-    this._appendAppBoot(result.directory, appFilePath);
+    this._updateAppFilesInManifest(result.directory);


Is there a way for us to only do this in Embroider land?

There's no obvious way and I don't really want to give one to people. Running Embroider and a classic build within the same broccoli system is a valid thing to want to do.

@ef4 any reasons for not to provide a way to check the embroider land. It will make it easier for Fastboot build to switch between classic build flow and changes needed to be made for embroider. Also, will help in isolating the build issue specific to embroider.

There's really no such thing as "outside" vs "inside" embroider. The addons all get instantiated before any embroider code even runs, and that is intentional. Also, the last time we exposed a global flag like this inside broccoli (the one for checking for fastboot) we regretted it, because it's just not really how broccoli works.

So no, I really don't want v1-formatted addons to start adding code to behave special under embroider. The only supported way to do that is to ship as a native V2 addon, after we merge RFC 507.

Also, this change is an improvement for all builds, not just embroider. It moves us in a better architectural direction.

Thank you @ef4. This makes sense.

rwjblue · 2019-06-17T15:30:08Z

index.js

@@ -19,6 +19,7 @@ const Funnel = require('broccoli-funnel');
 const p = require('ember-cli-preprocess-registry/preprocessors');
 const fastbootTransform = require('fastboot-transform');
 const existsSync = fs.existsSync;
+const cheerio = require('cheerio');


We should swap to using simple-html-tokenizer to parse, I'll try to whip up a demo to show what I mean.

dnalagatla · 2019-08-08T17:33:24Z

Hi @kratiahuja @tomdale @rwjblue @ef4, There is a test failure related to this test - https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/test/package-json-test.js#L232. This test is related to custom output paths configured in ember-cli-build.js

Here is the configuration of custom output path used for above test - https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/test/fixtures/customized-outputpaths/ember-cli-build.js#L5

Not sure how this scenario be handled in terms of index.html, should we expect the custom paths to be present in index.html. As I am not familiar with ember-cli set up for that. please let me know your thoughts.

rwjblue · 2019-08-09T16:27:55Z

Chatted with @dnalagatla and @mansona in today's FastBoot meeting, in order to work around that failing test we will need to keep the existing behaviors around as a fallback.

dnalagatla · 2019-08-09T17:57:08Z

Chatted with @dnalagatla and @mansona in today's FastBoot meeting, in order to work around that failing test we will need to keep the existing behaviors around as a fallback.

@rwjblue I made updates to keep the current low intact and only get the app files from index.html when .js file doesn't exist. Test related custom output paths is passing now.

dnalagatla · 2019-08-12T15:34:08Z

Rebased with master and resolved conflicts. @rwjblue please review when you get a chance

index.js

* Only update manifest when the current app file doesn't exist. * Providing flexibility to support embroider build and current build.

stefanpenner · 2019-09-11T20:25:37Z

@dnalagatla / @rwjblue this appears to have no tests, are there tests elsewhere?

stefanpenner · 2019-09-11T20:26:05Z

package.json

@@ -45,6 +45,7 @@
    "chai": "^4.1.2",
    "chai-fs": "^2.0.0",
    "chai-string": "^1.4.0",
+    "cheerio": "^1.0.0-rc.3",


I believe this should be a dependency not a devDependency

stefanpenner · 2019-09-12T01:10:28Z

This code does not appear to support rebuilds, when a rebuild occurs it appears to go back to not reading the index.html

dnalagatla force-pushed the dnalagatla/support-embroider-build branch from a6167f8 to 41e3da9 Compare June 5, 2019 19:44

dnalagatla changed the title ~~Added support reading app files form index.html in Manifest~~ Added support for embroider build and current build Jun 5, 2019

tomdale reviewed Jun 5, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

dnalagatla force-pushed the dnalagatla/support-embroider-build branch from 41e3da9 to 7f0e8b2 Compare June 5, 2019 20:10

dnalagatla force-pushed the dnalagatla/support-embroider-build branch from 7f0e8b2 to 29e27b9 Compare June 6, 2019 18:47

dnalagatla force-pushed the dnalagatla/support-embroider-build branch 2 times, most recently from 806dc92 to 20fb86a Compare June 15, 2019 16:14

dnalagatla changed the title ~~Added support for embroider build and current build~~ Added support to update manifest appfiles from index.html Jun 15, 2019

rwjblue reviewed Jun 17, 2019

View reviewed changes

dnalagatla force-pushed the dnalagatla/support-embroider-build branch 3 times, most recently from 282d6da to a0bcd90 Compare August 5, 2019 07:05

dnalagatla force-pushed the dnalagatla/support-embroider-build branch from a0bcd90 to b3619fe Compare August 9, 2019 17:24

dnalagatla force-pushed the dnalagatla/support-embroider-build branch 2 times, most recently from 7b8c564 to c277043 Compare August 12, 2019 15:32

rwjblue reviewed Aug 13, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

dnalagatla force-pushed the dnalagatla/support-embroider-build branch from c277043 to b3646cf Compare August 13, 2019 21:03

Added support to update manifest app files from index.html

0a12a6f

* Only update manifest when the current app file doesn't exist. * Providing flexibility to support embroider build and current build.

dnalagatla force-pushed the dnalagatla/support-embroider-build branch from b3646cf to 0a12a6f Compare August 13, 2019 21:15

rwjblue approved these changes Aug 13, 2019

View reviewed changes

rwjblue merged commit 87a8695 into ember-fastboot:master Aug 13, 2019

stefanpenner reviewed Sep 11, 2019

View reviewed changes

This was referenced Sep 11, 2019

cheerio is used outside of tests so must be a dependency #725

Merged

#701 appears to lack tests #726

Open

rwjblue added the enhancement label Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support to update manifest appfiles from index.html #701

Added support to update manifest appfiles from index.html #701

dnalagatla commented Jun 5, 2019 •

edited

dnalagatla commented Jun 5, 2019

tomdale commented Jun 5, 2019

rwjblue commented Jun 5, 2019

ef4 commented Jun 5, 2019

dnalagatla commented Jun 6, 2019

rwjblue commented Jun 6, 2019

ef4 commented Jun 6, 2019

rwjblue commented Jun 14, 2019

dnalagatla commented Jun 15, 2019 •

edited

rwjblue Jun 17, 2019

ef4 Jun 24, 2019

dnalagatla Aug 5, 2019 •

edited

ef4 Aug 5, 2019

dnalagatla Aug 5, 2019

rwjblue Jun 17, 2019

dnalagatla commented Aug 8, 2019 •

edited

rwjblue commented Aug 9, 2019

dnalagatla commented Aug 9, 2019

dnalagatla commented Aug 12, 2019

stefanpenner commented Sep 11, 2019

stefanpenner Sep 11, 2019

stefanpenner Sep 11, 2019

stefanpenner commented Sep 12, 2019

Added support to update manifest appfiles from index.html #701

Added support to update manifest appfiles from index.html #701

Conversation

dnalagatla commented Jun 5, 2019 • edited

dnalagatla commented Jun 5, 2019

tomdale commented Jun 5, 2019

rwjblue commented Jun 5, 2019

ef4 commented Jun 5, 2019

dnalagatla commented Jun 6, 2019

rwjblue commented Jun 6, 2019

ef4 commented Jun 6, 2019

rwjblue commented Jun 14, 2019

dnalagatla commented Jun 15, 2019 • edited

rwjblue Jun 17, 2019

Choose a reason for hiding this comment

ef4 Jun 24, 2019

Choose a reason for hiding this comment

dnalagatla Aug 5, 2019 • edited

Choose a reason for hiding this comment

ef4 Aug 5, 2019

Choose a reason for hiding this comment

dnalagatla Aug 5, 2019

Choose a reason for hiding this comment

rwjblue Jun 17, 2019

Choose a reason for hiding this comment

dnalagatla commented Aug 8, 2019 • edited

rwjblue commented Aug 9, 2019

dnalagatla commented Aug 9, 2019

dnalagatla commented Aug 12, 2019

stefanpenner commented Sep 11, 2019

stefanpenner Sep 11, 2019

Choose a reason for hiding this comment

stefanpenner Sep 11, 2019

Choose a reason for hiding this comment

stefanpenner commented Sep 12, 2019

dnalagatla commented Jun 5, 2019 •

edited

dnalagatla commented Jun 15, 2019 •

edited

dnalagatla Aug 5, 2019 •

edited

dnalagatla commented Aug 8, 2019 •

edited