Support CSS from CDN in esm-views #1757
Conversation
🦋 Changeset detectedLatest commit: 3e6d3e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| return ` | ||
| const link = document.createElement('link'); | ||
| link.rel = 'stylesheet'; | ||
| link.type = 'text/css'; |
There was a problem hiding this comment.
What about .less files?
There was a problem hiding this comment.
I don't think we're expecting to process any files coming from the CDN at runtime - the expectation is that the CDN provides files that are already processed by the build process of the dependency.
| link.rel = 'stylesheet'; | ||
| link.type = 'text/css'; | ||
| link.href = '${url}'; | ||
| document.getElementsByTagName('HEAD')[0].appendChild(link); |
There was a problem hiding this comment.
Edge case: the <head> tag is optional, so it may not be present. I suggest creating it if it doesn't exist. https://google.github.io/styleguide/htmlcssguide.html#Optional_Tags
There was a problem hiding this comment.
TL;DR: Done.
It seems that Webpack adds <head> even when we don't specify it in the template, but I added a check and replacement for missing head anyway. Better safe than sorry.
| : isEnvDevelopment, | ||
| }, | ||
| undefined, | ||
| true, |
There was a problem hiding this comment.
Should this be isEsmView?
There was a problem hiding this comment.
Done, thanks!
| }); | ||
| }); | ||
|
|
||
| describe('WHEN we specify an allow / block list', () => { |
There was a problem hiding this comment.
This test file is very long, and these unit tests may get lost. I think it would be better to put them in a file that makes it clear this is testing filterDependencies.ts.
There was a problem hiding this comment.
Factored out in partitionDependencies.test.ts. Why didn't I call it filterDependencies? Because we're testing only one method in the file (partitionDependencies) and the other one - that deals with env variable parsing - is already tested in esmView.test.ts
There was a problem hiding this comment.
That's fair enough, though I would have been happy if you'd created a to-be-completed test file as we can always flesh it out later!
| export function matchDependencies({ | ||
| packageDependencies, | ||
| // By default, everything in allow list | ||
| allowList = ['**'], |
There was a problem hiding this comment.
Re-reading this, I think "allow", "block", and even "matchDependencies" (and "filterDependencies") are not self-descriptive. Really what you're looking for is external vs bundled. It'll be very easy to forget what this function is trying to do or what it's for.
There was a problem hiding this comment.
@benpryke I changed terminology to reflect extarnal / bundled in the function that was introduced in this PR, and renamed it to partitionDependencies.
I didn't change it in the rest of the file, since EXTERNAL_BLOCK_LIST and EXTERNAL_ALLOW_LIST were approved months ago, are part of the user interface and not in scope of this PR.
Problem
Suppose we have a CSS import from an external dependency like:
In an
esm-view, we want to load the dependency from CDN instead of bundling it from local dependencies. In the future, we will also be able to use CSS Module Scripts to add styles to the page (and possibly deduplicate them).The problem, with Webpack, is that we can't just rewrite the import like we do with js modules. We need to use an helper function that fetches the css (or imports it with an assertion) from the CDN.
Solution
I used a Webpack pitching loader on top of our chain of style loaders. A pitching loader has a
pitchmethod that gets evaluated left-to-right in the array of loaders. If it returns something truthy, it prevents the subsequent loaders to run; otherwise, all the other loaders for the rule run right-to-left (so the top loader is last). It works in our case because:undefinedfrom the pitch function, letting the whole loader chain process it, and when our loader runs last, we just act as a bypass and return whatever output the loader chain has processed before us.The pros of this approach is that it uses the correct mechanism to load a dependency (a loader) and the solution is quite compact. The cons are that we re-create the import submodule "manually" (by looking at the Webpack relative path of the requested file) and that the loader is mandatorily positional (it must sit before the others).
Alternative solutions
Other
This PR also includes a bit more debug logging when filtering external dependencies and a small bugfix (wildcard value in allow list is
**instead of*to include scoped packages)