Skip to content
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

Enable Embroider #1

Merged
merged 12 commits into from
May 7, 2021
Merged

Enable Embroider #1

merged 12 commits into from
May 7, 2021

Conversation

thoov
Copy link
Owner

@thoov thoov commented Apr 29, 2021

text/0000-enable-embroider.md Outdated Show resolved Hide resolved

#### Fingerprinting

In classic builds there was a `app.options.fingerprint.exclude` configuration which users could configure to disable fingerprinting for specific files or `app.options.fingerprint.enabled` field to disable fingerprinting for all files. Under Embroider, JS and CSS assets will always be fingerprinted in production mode (all app code is always fingerprinted even in dev mode as they will become "chunks") except for any asset that contains the data attribute `data-embroider-ignore`. An example of this is that `testem.js` is [excluded from fingerprinting](https://github.com/ember-cli/ember-cli/blob/1301d24c8d7dec1f1559cffd0688d46bcd50284e/lib/broccoli/ember-app.js#L351-L355) as that explicit file name is expected. However, under embroider we can change `tests/index.html` to contain:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are other fingerprint options still supported?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently no. I can make this section state more explicitly that app.options.fingerprint are not enabled

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could (should?) point out that there is still quite a gap in the functionality, as Embroider will only fingerprint the basic JS and CSS files that it adds to index.html. broccoli-asset-rev would fingerprint arbitrary files, e.g. images, and rewrite all the references to those (as long as it can "see" the file paths, so they have to be literals, i.e. no dynamic concatenations logic or so), e.g in JS or CSS files. AFAIK there is no solution for this yet in Embroider-land.

Which is quite annoying as you cannot serve images with HTTP headers that enable long-term browser caching as you normally would. Or if you do, they are not invalidated when they change.

And people are probably quite used to this (the previous behavior), so when this does not work it would feel like a bug rather than a missing feature.

text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
Copy link

@acorncom acorncom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a few typos reading through this, thanks for the work on it!

text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
text/0000-enable-embroider.md Outdated Show resolved Hide resolved
@thoov
Copy link
Owner Author

thoov commented May 7, 2021

I am going to squash down these commits and open this in the upstream RFC repo. I will copy over the open comment so we don't lose it.

@thoov thoov merged commit dbb7a7c into master May 7, 2021
@thoov thoov deleted the enable-embroider branch May 7, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants