Skip to content
This repository has been archived by the owner on Jun 19, 2018. It is now read-only.

Add webpack configurator tools #20

Merged
merged 15 commits into from
Apr 22, 2018
Merged

Conversation

zetlen
Copy link
Contributor

@zetlen zetlen commented Mar 12, 2018

Adds two important utilities for development. Excerpted from READMEs:

ServiceWorkerPlugin

Webpack plugin for configuring a ServiceWorker for different PWA development
scenarios.

           new ServiceWorkerPlugin({
               env: {
                   mode: 'development'
               },

               paths: {
                   output: path.resolve(__dirname, 'web/js'),
                   assets: path.resolve(__dirname, 'web')
               },
               enableServiceWorkerDebugging: true,
               serviceWorkerFileName: 'sw.js',
               runtimeCacheAssetPath: 'https://cdn.url'
           })

Purpose

This plugin is a wrapper around the Google Workbox Webpack Plugin,
which generates a caching ServiceWorker based on assets emitted by Webpack.

In development, ServiceWorkers can cache assets in a way that interferes with
real-time editing and changes. This plugin takes configuration that can switch
between "normal development mode", where ServiceWorker is disabled, to "service
worker debugging mode", where ServiceWorker is enabled and hot-reloading.

Notes

  • MAGETWO-88754 Auto-disable service worker in dev mode
  • Did not end up having to fork Workbox, though we reserve the right in the future.
  • TODO: add automatic unregistration of SW
  • TODO: add default config values
  • TODO: make runtime caching optional

PWADevServer

Utility for configuring a development OS and a webpack-dev-server for PWA
development.

Usage

{
   devServer: await PWADevServer.configure({
          publicPath: '/pub/static/frontend/Vendor/theme/en_US/',
           backendDomain: 'https://magento2.localdomain',
           serviceWorkerFileName: 'sw.js',
           paths: {
               output: path.resolve(__dirname, 'web/js'),
               assets: path.resolve(__dirname, 'web')
           },
           id: 'magento-venia'
       })
}    

Purpose

The typical Webpack local development scenario uses the devServer settings in
webpack.config.js
to create
a temporary local HTTP server for showing edits in real time.

PWA development has a couple of particular needs:

  • The host must be secure and trusted to allow ServiceWorker operation
  • The host must be unique to prevent ServiceWorker collisions

Furthermore, Magento PWAs are Magento 2 themes running on Magento 2 stores, so
they need to proxy backend requests to the backing store in a customized way.

Notes

  • TODO: consolidate configuration
  • TODO: confirm Windows support
  • TODO: add default config values
  • TODO: add Firefox support

MagentoResolver

Adapter for configuring Webpack to resolve assets according to Magento PWA conventions.

       resolve: await MagentoResolver.configure({
           paths: {
               root: __dirname
           }
       })

Purpose

Generates a configuration for use in the resolve property of Webpack config.
Describes how to traverse the filesystem structure for assets required in source
files.

Currently, MagentoResolver does very little, but it's likely that the Magento
development environment will require custom resolution rules in the future; this
utility sets the precedent of the API for delivering those rules.

I added a lot of utilities and dependencies, and I'm sorry that this PR is so large as a consequence. This is part of a larger branch and the purpose of most of the separate utilities will become clear later.

@@ -0,0 +1,56 @@
## MagentoResolver

Adapter for configuring Webpack to resolve assets according to Magento PWA conventions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest rewording to:

An adapter that configures Webpack to resolve assets according to Magento PWA conventions.

Is there documentation for the Magento PWA conventions? If so, we should link to it.

config that uses it must use the [Exporting a Promise configuration type](https://webpack.js.org/configuration/configuration-types/#exporting-a-promise).
The newer `async/await` syntax looks cleaner than using Promises directly.

### Purpose
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose moving this section above Usage to make the information flow better: conceptual -> example -> API reference

```


- ⚠️ `MagentoResolver.configure()` is async and returns a Promise, so a Webpack
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest rewording to:

ℹ️ Note: MagentoResolver.configure() is asynchronous and returns a Promise. For more information, see the Webpack documentation about Exporting a Promise configuration type.

In the example provided, the newer async/await syntax is used because it is a cleaner alternative to using Promises directly.


### Purpose

Generates a configuration for use in the [`resolve` property of Webpack config](https://webpack.js.org/configuration/resolve/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested rewording:

This class generates a configuration object for the resolve property of a Webpack config file. The configuration object describes how Webpack should traverse the filesystem structure to retrieve assets required in source files.


### API

`MagentoResolver` has only one method: `.configure(options)`. It returns a Promise
Copy link
Contributor

Choose a reason for hiding this comment

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

The information in this paragraph is already described by the API or elsewhere on the page. It should be safe to remove this paragraph.

};
```

### Purpose
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose moving this section above Usage to make the information flow better: conceptual -> example -> API reference


### Purpose

This plugin is a wrapper around the [Google Workbox Webpack Plugin](https://developers.google.com/web/tools/workbox/guides/generate-service-worker/),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest splitting this into two sentences:

This plugin is a wrapper around the Google Workbox Webpack Plugin. It generates a caching ServiceWorker based on assets emitted by Webpack.

which generates a caching ServiceWorker based on assets emitted by Webpack.

In development, ServiceWorkers can cache assets in a way that interferes with
real-time editing and changes. This plugin takes configuration that can switch
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing the second sentence to:

This plugin can be configured to run in the following modes:

  • normal development - ServiceWorker is disabled
  • service worker debugging - ServiceWorker and hot-reloading are enabled


### API

`PWADevServer` has only one method: `.configure(options)`. It returns a Promise
Copy link
Contributor

Choose a reason for hiding this comment

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

Information in this paragraph is already mentioned elsewhere on this page. It can be safely deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we replace this whole readme with a link to devdocs?


### API


Copy link
Contributor

Choose a reason for hiding this comment

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

My attempt at cleaning up the API format:

ServiceWorkerPlugin

ServiceWorkerPlugin(options: PluginOptions): Plugin
Plugin constructor for the ServiceWorkerPlugin class.

PluginOptions

env: Object (Required)
An object that represents the current environment.

  • env.mode: String (Required)
    Must be either 'development' or 'production'.

paths: Object (Required)
The local absolute paths to theme folders.

  • paths.assets: String
    The directory for public static assets.

enableServiceWorkerDebugging: Boolean
When true, hot reloading is enabled and the ServiceWorker is active in the document root, regardless of the publicPath value.
When false, the ServiceWorker is disabled to prevent cache interruptions when hot reloading assets.

serviceWorkerFileName: String (Required)
The name of the ServiceWorker file this theme creates.
Example: 'sw.js'

runtimeCacheAssetPath: String (Required)
A path or remote URL that represents the root path to assets the ServiceWorker should cache as requested during runtime.

@ericerway ericerway added this to the Milestone 2 milestone Mar 30, 2018
@zetlen
Copy link
Contributor Author

zetlen commented Apr 2, 2018

@jcalcaben At long last, here's some documentation, edited down to only contain what's in this PR. Let me know what I can do to help move this long!

README.md Outdated
Divides static assets into bundled "chunks" based on components registered
with the Magento PWA `RootComponent` interface
- [`PWADevServer`](docs/PWADevServer.md) -- Configures your system settings and
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of this sentence is missing

README.md Outdated
1. Install Webpack Webpack Dev Server, and Buildpack as developer dependencies.

```sh
npm install --save-dev webpack webpack-dev-server @magento/pwa-buildpack
Copy link
Contributor

Choose a reason for hiding this comment

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

I think webpack-cli is also needed as a dependency

@zetlen zetlen force-pushed the zetlen/add-webpack-configurators branch 2 times, most recently from e6364e9 to d2aa782 Compare April 6, 2018 17:51
async del(...keyparts) {
const db = await this.constructor.db();
const key = this.makeKey(keyparts);
return new Promise((wow, ow) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would appreciate if we renamed all of these in the PR to res/rej or resolve/reject. It's cute, but it's not super professional in this codebase, and just means you have to trace more code to figure out what these mean.

Copy link

Choose a reason for hiding this comment

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

Agree, I really overlooked this line.

@@ -0,0 +1,39 @@
const lget = require('lodash.get');
const NOPE = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename to describe what this is used for.


### Intro
## Quick Setup
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcalcaben to confirm, is this the stuff you're moving to devdocs? Seems slightly out of place in this repo

Copy link
Contributor

Choose a reason for hiding this comment

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

@DrewML
Yes, this content is moving to the pwa-devdocs repo.
See related Pull Request: magento-research/pwa-devdocs#20

package.json Outdated
"test:dev": "run-s prettier lint jest:ci",
"test:watch": "jest --watch --runInBand",
"test:debug": "node --inspect-brk ./node_modules/.bin/jest --runInBand",
"test:watch:focus": "jest --watch --runInBand --coverageReporters=text --collectCoverageFrom=\"${FOCUS}/**/*.js\" --no-cache \"${FOCUS}.*\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment to another repo - don't want to overwhelm contributors with a billion options. This would be a good candidate for a local config

@@ -1,5 +1,8 @@
module.exports = {
parserOptions: {
sourceType: 'module'
},
rules: {
'node/no-unsupported-features': 'off'
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment would be helpful to provide context on why we're disabling this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary fix anyway, since the ExtensionComponentWrap is supposed to move to Peregrine. Added a comment explaining that.

src/index.js Outdated

module.exports = {
babelPluginMagentoLayout,
MagentoRootComponentsPlugin
Webpack: require('./webpack')
Copy link
Contributor

Choose a reason for hiding this comment

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

Webpack isn't a constructor, so it would make sense to follow JS conventions and use lowercase

Copy link

Choose a reason for hiding this comment

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

True. Also keep in mind that this object will be imported by webpack.config.js, where webpack will already be present as the logical import from webpack itself.

Maybe just call this buildpack, after the module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DrewML I don't think it's established JS conventions that PascalCase only applies to constructors. As a quick sample, Airbnb applies PascalCase much more broadly.

@jimbo Very true that we have to find ways to make this not collide with common stuff in webpack.config.js. I can't rename it buildpack, because this grouping is specifically for objects that work in the Webpack lifecycle, be they configurators or plugins or loaders.

I think I'm going to rename it to WebpackTools. I'm also going to say we should use PascalCase for all singleton objects, not just constructors. I understand that this decision is arbitrary, but I'm considering the many PHP developers who will be reading lots of JS for the first time. The distinction between a constructor and a plain object might not be immediately clear, and the capitalization rules should appear typographically consistent and familiar.

myDebug.here(__dirname).sub('extra');
expect(debug.mock.calls[1][0]).toBe('pwa-buildpack:util:__tests__:extra');
});
afterEach(() => jest.resetAllMocks());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Would be nice to see this next to the beforeEach - I originally started a comment saying this leaked a mock because I hadn't scrolled.

return logger;
};
module.exports = {
here(p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a more descriptive name for this? setDebugFile or something. A single adverb doesn't really say much

const root = path.resolve(__dirname, '../');
const pkg = require(path.resolve(root, '../package.json'));
const toolName = pkg.name.split('/').pop();
const tokenize = (...parts) => parts.join(':');
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like the opposite of tokenizing 🤔

debug(`no cached db exists, pulling db from ${dbFilePath}`);
const db = flatfile(dbFilePath);
debug(`db created, waiting for open event`, db);
db.on('open', () => {
Copy link
Contributor

@DrewML DrewML Apr 11, 2018

Choose a reason for hiding this comment

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

What about db.on('error')? Seems like we don't handle that

);
}
if (keyparts.length === 0) {
// a scalar, then
Copy link
Contributor

@DrewML DrewML Apr 11, 2018

Choose a reason for hiding this comment

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

I know what a scalar is, but I'm not sure what this comment is trying to convey to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bad and confusing corner case for the GlobalConfig object, which acted like a single-value store if the key function had no parameters. I've removed it!

return resolve(homedir(), './.config/pwa-buildpack.db');
}
static async db() {
if (!this._dbPromise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this in a static method is a reference to the class itself. If we're storing state that is shared between instances on the class itself, this feels like a singleton. Is there ever a scenario where > 1 GlobalConfig instance should be alive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! GlobalConfig instances share an underlying flat file database, but each instance of GlobalConfig has a different key prefix and key generator; this way, we can make separate namespaces without littering the user's FS with files. If it feels gross to make that a static method, I can instead make the db connection into a separate file that exports a singleton. Would that feel better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah it's fine - just wanted to understand.

async values(xform = x => x) {
const db = await this.constructor.db();
return db.keys().reduce((out, k) => {
if (k.indexOf(this._prefix) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

String.prototype.startsWith would be the more modern way to do this

@@ -0,0 +1,11 @@
const { promisify } = require('util');
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these can just be replaced with pify(moduleRef) using https://github.com/sindresorhus/pify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per sindresorhus/pify#41 I think we should stick with native util.promisify, since it works with the util.promisify.custom symbol. That gives us a standard way we can normalize Node APIs with multi-argument callbacks, and the Node documentation reflects this. I like pify, but its behavior with things like child_process.exec is now nonstandard, so I think we should stay with the native here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! TIL

icns: path.join(__dirname, '../../buildpack-logo.icns')
};

const elevate = async cmd =>
Copy link
Contributor

Choose a reason for hiding this comment

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

elevate what? Name doesn't describe the purpose without reading through the implementation

});
});

module.exports = async (fn, ...args) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this function needs a description. I can't figure out why we're spitting out a to-stringed fn to a tmp file

- The host must be _secure_ and _trusted_ to allow ServiceWorker operation
- The host must be _unique_ to prevent ServiceWorker collisions

urthermore, Magento PWAs are Magento 2 themes running on Magento 2 stores, so
Copy link
Contributor

Choose a reason for hiding this comment

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

urthermore and hey are missing letters


#### `options`

- `id: string`: **Required.** A unique ID for this project. Theme name is
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be useful to document what this id is used for (related to my other comment)

/* other plugins */
new ServiceWorkerPlugin({
env: {
mode: 'development'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just development or production? If so, as a plugin, the code already has access to read webpack 4's mode setting (required field in config). So we don't need to manually specify here. Less chance for bugs to creep in from bad configs

Copy link
Contributor

Choose a reason for hiding this comment

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

Believe the config will be available on compiler.options in your plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DrewML Have you been moving everything to Webpack 4? I got bit by a breaking API change this morning, which reminded me that we ought to probably do that carefully.

Also, the mode could be other values, like test or stage.

Copy link
Contributor

@DrewML DrewML Apr 17, 2018

Choose a reason for hiding this comment

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

We're very close - I need to review and merge the open PR from #21 and do some testing.

We don't have to switch to mode until then, but I'd like to use a different term in our plugin so we're not overloading what mode means in a webpack config (mode in webpack 4 only accepts 2 values).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about phase?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return {
modules: [options.paths.root, 'node_modules'],
mainFiles: ['index'],
extensions: ['.js']
Copy link
Contributor

Choose a reason for hiding this comment

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

This is overriding webpack's default .json entry, which is going to surprise anyone who is used to JSON imports working with node/webpack OOTB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, whoops!

}),
hostnamesById: new GlobalConfig({
prefix: 'devhostname-byid',
key: x => x
Copy link
Contributor

Choose a reason for hiding this comment

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

Can key just default to an identity func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a footgun because key has to return a string, but I can make its default Object.prototype.toString.call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, or could just default to String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched it to toString

}
}

module.exports = (name, simpleSchema) => (callsite, options) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use joi or JSON schema here? I'd imagine you're going to hit the edges of this pretty quick, and we'll quickly end up in the business of writing an options validator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No good reason! I had always intended to, but I was moving fast. Seems like tech debt we could pay off later, though; do you want to hold up merge until we switch to joi?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to hold up, but a comment with a TODO would be cool. Easy "good first issue" to throw up later

Copy link
Contributor Author

@zetlen zetlen Apr 17, 2018

Choose a reason for hiding this comment

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

Good idea

package.json Outdated
"test:dev": "jest --watch",
"jest:ci": "jest -i --forceExit --testResultsProcessor=./node_modules/jest-junit-reporter",
"test:dev": "run-s prettier lint jest:ci",
"test:watch": "jest --watch --runInBand",
Copy link
Contributor

Choose a reason for hiding this comment

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

--runInBand is going to slow down watching in development - doubt that was the intention here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I meant for that to be in a debug and not a watch.

await unlink(scriptLoc);
throw e;
}
// do a finally
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, gotta update that comment. I had discovered that the semantics of finally are somewhat different with async/await. Here is a demo I cooked up about it.

And a relevant GitHub comment about same.

Given the confusion I thought I should avoid finally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had discovered that the semantics of finally are somewhat different with async/await.

I'm confused - your example shows that try/catch/finally works the exact way I'd expect. It's Promise.prototype.finally that can be confusing, but you're using async/await here so I'm not quite grokking how that plays into this?

Can you explain what you're wanting to do in a finally block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Welp, I'm an idiot. I got the behavior backwards in my head--the finally is fine. (I just wanted to delete a temp file)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, not an idiot! Just the rush to Imagine

Here's a cool doggo

doggo

@DrewML
Copy link
Contributor

DrewML commented Apr 17, 2018

I pushed a commit to your branch to fix CI

PWADevServer.findFreePort()
]);

await PWADevServer.hostnamesById.set(id, hostname);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these 2 await calls run in parallel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember, but since they were both touching the flat file I felt like playing it safe.

return maybeHostname;
} else {
debug(`findFreeHostname: ${maybeHostname} bound to port`, exists);
return PWADevServer.findFreeHostname(identifier, times + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a max # of times and a bail out

GlobalConfig.mockImplementation(({ key }) => ({
set: jest.fn((...args) => {
const keyParts = args.slice(0, -1);
expect(typeof key(...keyParts)).toBe('string');
Copy link
Contributor

Choose a reason for hiding this comment

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

It's strange to me to have an assertion inside of a mock. Think it would make more sense to be checking .calls in the test

@@ -0,0 +1,60 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we going to have misses with this strategy? background-image in external stylesheets is the first that comes to mind, but I have to assume there are more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are more for sure. This is a stopgap until we can hack Magento Framework to have branch logic in their asset URL resolvers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like what you just mentioned would be useful in this prelude

} finally {
await unlink(scriptLoc);
}
// do a finally
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant to remove this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did a finally is rememberinged.

i was raised by a cup of coffee

@zetlen
Copy link
Contributor Author

zetlen commented Apr 22, 2018

If this passes CI, I'm gonna merge this before full approval and we can resolve issues later--it's important for demo and community purposes to have this functionality there.

@zetlen zetlen removed request for rowan-m and jimbo April 22, 2018 18:27
@zetlen zetlen merged commit 399c00e into master Apr 22, 2018
@DrewML DrewML mentioned this pull request May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants