[WIP] embroider work does not currently support rebuilds, this … #727

stefanpenner · 2019-09-12T02:00:37Z

fix rebuild
add tests
drop cheerio in-favor of simple-html-tokenizer
confirm fixes fastboot support + tests embroider-build/embroider#322
work through the inline todos

stefanpenner · 2019-09-12T05:04:58Z

lib/embroider/update-manifest-from-html.js

+const fs = require('fs');
+const findScriptSrcs = require('find-scripts-srcs-in-document');
+// TODO: review this
+const FILES_TO_SKIP = /vendor|vendor-static/gi;


look at this

We cneed to have specific attributes we look for that indicate "vendor" vs "app code" vs "ignore this script".

Why does fastboot need to distinguish vendor vs app?

As far as "ignore this script", we already have the ability to put fastboot guards around any code that shouldn't run in fastboot. I would rather stick with one technique like that than add yet another API here. And guards are more like the proposed @embroider/macros, which has the nice property of letting us produce a single build that works in-browser and in-fastboot during development, while producing optimized separate builds in prod.

I haven't dug in yet, still investigating this one. (This was a port of the existing code, with fixes and tests)

the "look at this" comment was more for myself, sorry I should have been clear.

stefanpenner · 2019-09-12T05:05:19Z

lib/embroider/update-manifest-from-html.js

+  const pkg = JSON.parse(fs.readFileSync(root + '/package.json', 'UTF8'));
+
+  // bail if not embroider (let's chat if this is appropriate);
+  if (!('version' in pkg['ember-addon'])) {


@ef4 / @rwjblue thoughts on a more explicit check?

I'd prefer to continue to use any existing content in pkg.fastboot.manifest.appFiles (this supports non-embroider users) and only parse from index.html in Embroider builds. We can work towards removing that (conceptually), but it seems safer for now for existing consumers that are not experimenting with Embroider.

The issue is, we mutate said file' appFiles (and the build may choose to as well). So it's contents cannot be trusted.

stefanpenner · 2019-09-12T05:05:42Z

lib/embroider/update-manifest-from-html.js

+
+  // otherwise, we must parse the index.html file to figure out what's next
+  const indexHtml = fs.readFileSync(root + '/index.html', 'UTF8')
+  // TODO: fix simple-html-tokenizer: https://github.com/tildeio/simple-html-tokenizer/pulls


tildeio/simple-html-tokenizer#71

* ensure rebuilds work correctly * use simple-html-tokenizer instead of cheerio (less dependencies, same parser as we use everywhere else, and faster) * detect embroider usage more directly (we should discuss) * tests

stefanpenner · 2019-09-12T05:08:40Z

lib/embroider/update-manifest-from-html.js

+
+  // otherwise, we must parse the index.html file to figure out what's next
+  const indexHtml = fs.readFileSync(root + '/index.html', 'UTF8')
+  // TODO: fix simple-html-tokenizer: https://github.com/tildeio/simple-html-tokenizer/pull/71


fix simple-html-tokenizer: tildeio/simple-html-tokenizer#71

stefanpenner · 2019-09-12T05:08:48Z

lib/embroider/update-manifest-from-html.js

+
+  const appFiles = findScriptSrcs(indexHtml, findScriptSrcs.ignoreWithAttribute('data-embroider-ignore'))
+    // TODO: these FILES_TO_SKIP things seem over zealous
+    .filter( src => !FILES_TO_SKIP.test(src))


feels overzealous

stefanpenner · 2019-09-12T05:09:00Z

lib/embroider/update-manifest-from-html.js

+    // TODO: these FILES_TO_SKIP things seem over zealous
+    .filter( src => !FILES_TO_SKIP.test(src))
+    // TODO: handle prefixed / custom origin paths / or files
+    .filter(src => fs.existsSync(`${root}/${src}`));


handle prefixed/custom origin path etc.

stefanpenner · 2019-09-12T05:09:12Z

lib/embroider/update-manifest-from-html.js

+    .filter(src => fs.existsSync(`${root}/${src}`));
+
+  // TODO: this feels super janky, we need to figure out a better approach
+  const fastbootFile = pkg.fastboot.manifest.appFiles.find(x => /-fastboot.js$/.test(x))


this feels risky, we should find a better approach

stefanpenner · 2019-09-12T05:09:49Z

lib/embroider/update-manifest-from-html.js

+'use strict';
+
+const fs = require('fs');
+const findScriptSrcs = require('find-scripts-srcs-in-document');


tested lib that uses simply-html-tokenizer

stefanpenner · 2019-09-12T05:10:11Z

test/embroider/update-manifest-from-html.js

+const fs = require('fs');
+const updateManifestFromHtml = require('../../lib/embroider/update-manifest-from-html')
+
+describe('embroider/update-manifest-from-html', function() {


TODO: more tests

dnalagatla · 2019-09-19T01:50:51Z

lib/embroider/update-manifest-from-html.js

+  const addon = pkg['ember-addon'];
+
+  // bail if not embroider (let's chat if this is appropriate);
+  if (typeof addon !== 'object' || !('version' in addon)) {


I am also, curious to know if there is a better option for the check regarding embroider or classic.

Addons doing different things under embroider is a bad idea.

The whole point of @embroider/compat is to support addons that don't know about embroider. If an addon wants to actually support embroider, it needs to ship as a v2 addon, thus avoiding @embroider/compat entirely and only using supported APIs in @embroider/core.

If an addon wants to actually support embroider, it needs to ship as a v2 addon

Which is actually impossible since V2 isn't actually shipped yet 😉

. If an addon wants to actually support embroider, it needs to ship as a v2 addon, thus avoiding @embroider/compat entirely and only using supported APIs in @embroider/core

Could be a dumb question, but just curious - what can be done to use V2 but still be backward compatible. I think Updating ember-cli-fastboot AddOn to use V2 would need Major version bump. And have to maintain both versions. I don't think that is scalable.

@rwjblue please correct me if I am wrong.

I am very tempted to use process.env.EMBROIDER and update the documentation to indicate, if we have to use embroider with fastboot then run EMBROIDER=true ember s. And use that property to differentiate embroider and classic flow.

I am not happy with this approach but will cover our requirement and since embroider is an experimental build I think this will be ok.

any thoughts?

I updated the PR - #729, with changes related to using process.env.EMBROIDER property to check if this is embroider flow or not.

@dnalagatla it won't require a major bump, v2 addons can also be valid v1 addons because ember-cli respects the ember-addon.main key in package.json, which lets us distinguish the v1 entrypoint from the v2 build entrypoint (ember-addon.build) and the v2 runtime entrpoint (main).

@rwjblue there's nobody better positioned than the people in this thread to make v2 addons become fully implemented. There's not a lot of work required, since the whole system is already designed around their needs. But that is all beside my point: these changes aren't breaking as far as I know, if they're breaking show me how and we can adjust until they're not breaking. If you just want a feature flag inside ember-cli-fastboot to manage the rollout, fine, but don't name it "EMBROIDER" because that's just confusing. It's an ember-cli-fastboot specific feature flag.

@ef4 There is a difference between the classic build and embroider build flow, as a result I was interested in having that flag:

In Classic build flow the following are the steps occurs in ember-cli-fastboot build -

Ember-cli gives a flag to turn off autoBoot by setting this property app.options.autoRun and is set to false by default in include hook in ember-cli-fastboot - https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/index.js#L73

Then in contentFor hook app-boot type is used to define custom app-boot logic that can support Browser and Fastboot (using appFactory). - https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/index.js#L110

Embroider build doesn't support app-boot hook in contentFor to include custom app-boot logic needed for ember-cli-fastboot. - embroider-build/embroider#183

As app-boot is not supported, we introduced a flag to disable-auto-boot in fastboot, but for that flag to be effective we would have to set app.options.autoRun to true. That change is only compatible with embroider flow but not with classic build flow of ember-cli-fastboot.

cc: @stefanpenner

Hi @ef4, are there any plans to support app-boot in embroider.

dnalagatla · 2019-09-20T22:35:41Z

This PR can be closed as all the necessary changes for fixing embroider flow is in this PR - #729

Also, above PR fixes embroider related tests not running as part of yarn test and fixed tests based on the expected structure of the output directory.

@stefanpenner @rwjblue

ef4 · 2019-09-24T17:57:42Z

Yes, I think it would be a good idea to implement app-boot.

…

On Tue, Sep 24, 2019 at 1:48 PM Dinesh Nalagatla ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/embroider/update-manifest-from-html.js <#727 (comment)> : > @@ -0,0 +1,39 @@ +'use strict'; + +const fs = require('fs'); +const findScriptSrcs = require('find-scripts-srcs-in-document'); + +// TODO: hard coding these seem unfortunate, especially vendor-static which is non-standard +const FILES_TO_SKIP = /vendor|vendor-static/gi; + +module.exports = function updateManifestFromHtml(root) { + const pkg = JSON.parse(fs.readFileSync(root + '/package.json', 'UTF8')); + const addon = pkg['ember-addon']; + + // bail if not embroider (let's chat if this is appropriate); + if (typeof addon !== 'object' || !('version' in addon)) { Hi @ef4 <https://github.com/ef4>, are there any plans to support app-boot in embroider. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727?email_source=notifications&email_token=AACN6MTLZNBMGFXU7RYQ3WTQLJHF5A5CNFSM4IV42BM2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFYLEPQ#discussion_r327750242>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACN6MQMUOFLIKJHKATW57LQLJHF5ANCNFSM4IV42BMQ> .

stefanpenner mentioned this pull request Sep 12, 2019

fastboot support + tests embroider-build/embroider#322

Closed

8 tasks

stefanpenner force-pushed the fix-embroider branch 2 times, most recently from c5fff39 to 474a680 Compare September 12, 2019 05:01

stefanpenner changed the title ~~[Quick fix] embroider work does not currently support rebuilds, this …~~ [WIP] embroider work does not currently support rebuilds, this … Sep 12, 2019

stefanpenner commented Sep 12, 2019

View reviewed changes

stefanpenner force-pushed the fix-embroider branch from 474a680 to 9a366aa Compare September 12, 2019 05:07

Fix embroider usage

b262446

* ensure rebuilds work correctly * use simple-html-tokenizer instead of cheerio (less dependencies, same parser as we use everywhere else, and faster) * detect embroider usage more directly (we should discuss) * tests

stefanpenner force-pushed the fix-embroider branch from 9a366aa to b262446 Compare September 12, 2019 05:08

stefanpenner commented Sep 12, 2019

View reviewed changes

stefanpenner mentioned this pull request Sep 16, 2019

Create embroider branch mike-north/ember-octane-workshop#39

Open

handle missing ember-addon property in package.json gracefully

8e0d387

dnalagatla reviewed Sep 19, 2019

View reviewed changes

stefanpenner mentioned this pull request Sep 19, 2019

Changes to support embroider build for Fastboot Apps #729

Closed

stefanpenner closed this Sep 25, 2019

stefanpenner deleted the fix-embroider branch September 25, 2019 15:47

[WIP] embroider work does not currently support rebuilds, this … #727

[WIP] embroider work does not currently support rebuilds, this … #727

Uh oh!

Conversation

stefanpenner commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnalagatla Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnalagatla commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ef4 commented Sep 24, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stefanpenner commented Sep 12, 2019 •

edited

Loading

rwjblue Sep 19, 2019 •

edited

Loading

dnalagatla Sep 23, 2019 •

edited

Loading

dnalagatla commented Sep 20, 2019 •

edited

Loading