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

Use isModern checks to decide which arch assets to put in the app.man… #9776

Closed
wants to merge 4 commits into
base: release-1.7
from

Conversation

Projects
None yet
2 participants
@CaptainN
Contributor

CaptainN commented Mar 28, 2018

This PR does two things:

  1. It uses the isModern from the meteor/modern-browsers package to output content to the app.manifest based on the requesting browser.
  2. For legacy browsers, it avoids outputing dynamic .map files. (Which also addresses the TODO note referencing PR #9439)

A couple of other notes/questions:

  1. In the isDynamic method, we are checking to make sure .map files are dynamic to avoid putting them in the app.manifest - but should we just always avoid putting .map files in the manifest?
  2. This one may be important. I'm not certain the static content from the /public directory is listed correctly in WebApp.clientPrograms['web.browser.legacy'].manifest. Assets from that location contain the /__browser.legacy/ prefix, but those URLs don't resolve. Additionally, those /public/ URLs do not match the links in the NETWORK section below, which uses a different method to populate its list. I imagine the fix for this is somewhere upstream of this package.
Use isModern checks to decide which arch assets to put in the app.man…
…ifest, and add filter for web.browser.legacy in isDynamic check

@benjamn benjamn added this to the Release 1.6.2 milestone Mar 28, 2018

@benjamn

This comment has been minimized.

Member

benjamn commented Mar 28, 2018

In one early implementation of the modern/legacy system, we were adding /__browser/... to modern asset URLs and /__browser.legacy/... to legacy asset URLs, but we realized that would be a breaking change for most apps that currently load assets from /public without a prefix, so we decided to remove the /__browser/... prefix for modern assets in this commit. To answer your last question, I think it would be safe to exclude all legacy assets from the manifest, since we're going to keep encouraging apps to load assets without a prefix.

@benjamn

This comment has been minimized.

Member

benjamn commented Mar 28, 2018

should we just always avoid putting .map files in the manifest?

Yes, I would strongly support this approach, since source maps are not necessary for offline (non-development) usage, and the App Cache has a low maximum size (~5MB).

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Mar 28, 2018

I was thinking the way to deal with /public assets in the legacy build is to avoid using the /__browser.legacy prefix altogether for those specific assets, since they are still addressable from the root. Do you know where those are added to the manifest array?

Without changing that upstream, we could strip out the legacy prefix, if there is a way to detect that the items came from the /public/ location.

Some legacy browsers do support AppCache (IE11 for example) but not async/await. Removing public assets (and legacy built assets - which should work now) from the app.manifest altogether would mean appcache no longer supports those browsers.

@benjamn

This comment has been minimized.

Member

benjamn commented Mar 28, 2018

A legacy client can still request modern resources from the web server, so it should be fine to put modern asset URLs in the manifest exposed to legacy clients. When I said it would be safe to exclude legacy assets from the manifest, I didn't mean we shouldn't put any assets in the manifest for legacy clients. I think we should use the same (modern) asset URLs for both modern and legacy clients.

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Mar 28, 2018

Ah, okay that was my misunderstanding.

I figured my challenge then was to find a way to determine which assets in WebApp.clientPrograms['web.browser.legacy'].manifest came from the public directory.

I used inference:

{ path: 'app/bw-icon.svg',
   where: 'client',
   type: 'asset',
   cacheable: false,
   url: '/bw-icon.svg',
   size: 4579,
   hash: '1413cd9a7131f878ee4c6aa09596235758d7b329' }

For assets from /public the path seems to always start with "app/" - and the url starts with /__browser.legacy/ and would match the remaining path. This works in my limited testing (I pushed it in the PR), but it would be better if there was some way to know for certain that a particular asset came from the /public folder.

On the other hand, certain other assets (/webapp-manifest.json in particular, which is also prefixed in the manifest array) seem to benefit from the way I'm doing this now. I just can't say with 100% certainty that it'll work for every asset, nor can I really explain why it's working for additional files. (Er, nm this - webapp-manifest.json is in my own /public folder - I had to run the converted url through RoutePolicy.classify(url) to get that to not output. I'm not confident I know why that is yet)

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Mar 29, 2018

I tested this on a few more apps I have laying around, including a blaze app, and it seems to work well in at least my local set.

I also took a look upstream, to see if there is a better way to remove the prefixes from assets which come from the /public directory. It looks like the manifest get assembled at build time in /tools/isobuild/bundler.js line 668. It looks like this:

  // Also allow a special second suffix that will *only* be postpended to the
  // url, useful for query parameters.
  setUrlToHash(fileAndUrlSuffix, urlSuffix) {
    urlSuffix = urlSuffix || "";
    this.url = this.urlPrefix + "/" +
      this.hash() + fileAndUrlSuffix + urlSuffix;
    this.cacheable = true;
    this.targetPath = this.hash() + fileAndUrlSuffix;
  }

If there is some way to know that the file came from /public at this point, we can skip prepending the prefix altogether, and then we don't have to rewrite URLs at all in this package.

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Apr 3, 2018

I found the place in bundler.js to make the change, so that it doesn't include the prefix for /public assets (and assets which come from packages). This may also help with issue #9782

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Apr 4, 2018

I could use some help figuring out the failing unit tests. I assume that removing the prefix from /public assets has cause some of those to fail, but I wasn't sure where to look in there.

@benjamn

This comment has been minimized.

Member

benjamn commented Apr 6, 2018

@CaptainN Most of those failures are either known-flaky tests or timeouts, and the one that looks legitimate actually passes locally for me, so I've just triggered a rerun of the failing tests. 🤞

@benjamn benjamn self-assigned this Apr 18, 2018

@abernix abernix changed the base branch from release-1.6.2 to release-1.7 Apr 19, 2018

@abernix abernix referenced this pull request Apr 19, 2018

Merged

Release 1.7 #9826

benjamn added a commit that referenced this pull request Apr 27, 2018

Tolerate assets added with same path to both modern and legacy archit…
…ectures.

Since legacy assets are no longer disambiguated from modern assets with a
/__browser.legacy/ URL prefix (#9776), we need a principled way to handle
collisions in the staticFiles registry.
@benjamn

This comment has been minimized.

Member

benjamn commented Apr 27, 2018

Manually merged this PR into release-1.7 with the following additional commits:

Thanks for working on this, and thanks in advance for testing the next 1.7 beta! 😉

@benjamn benjamn closed this Apr 27, 2018

@benjamn

This comment has been minimized.

Member

benjamn commented Apr 28, 2018

We seem to have a slight problem (surfaced in #9846): legacy/Cordova CSS files are not assets, so their URLs still have the /__browser.legacy/ or /__cordova/ prefix. However, CSS often refers to assets like images and fonts in the public/ directory using relative URLs, which the browser interprets as relative to the location of the CSS file. In other words, it seems important for the asset URLs to continue to have the same prefix as the CSS file, since not everyone is using absolute asset URLs in their stylesheets.

If we go back to adding a prefix to asset URLs (as I think we must), we still have the problem of making sure non-prefixed asset URLs get included in the appcache manifest somehow, in case the non-prefixed URL gets loaded while offline by a legacy or Cordova browser. When you're connected to the network, this just works because the web.browser bundle provides the non-prefixed URLs, but it might risky to put all web.browser asset URLs into the legacy/Cordova manifests, due to appcache size limits.

Update: I was able to use the FALLBACK section in the app.manifest file to map un-prefixed asset URLs to their prefixed equivalents, so we can keep using prefixed URLs by default (which fixes #9846, I think), while also allowing un-prefixed URLs to be loaded offline: 7e883b3

benjamn added a commit that referenced this pull request Apr 28, 2018

benjamn added a commit that referenced this pull request Apr 28, 2018

Register app.manifest FALLBACK for non-prefixed asset URLs.
This is an alternate solution to the problems #9776 was intended to fix.
@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Apr 28, 2018

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Apr 28, 2018

Alternatively we will have to rewrite all the links a user adds to their sources and things like that using a Babel plugin or something

@benjamn

This comment has been minimized.

Member

benjamn commented Apr 29, 2018

@CaptainN I hear what you're saying, and I'm increasingly feeling like the /__browser.legacy/... URL prefixing was a mistake, even if we can sort of make it work by pretending that prefix-free web.browser asset URLs are safe for legacy clients to load.

If we want to remove the /__browser.legacy/ prefixes completely (which would simplify all this logic considerably), I think we just need to teach webapp how to serve different static files based on isModern(request.browser), which is something we already have to do without the help of URL prefixes in the dynamic-import package here.

Here's a PR that attempts to remove legacy prefixes: #9849

benjamn added a commit that referenced this pull request May 3, 2018

benjamn added a commit that referenced this pull request May 3, 2018

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