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

chore: Remove devtools and switch to pwa-buildpack #35

Closed
wants to merge 2 commits into from

Conversation

zetlen
Copy link
Contributor

@zetlen zetlen commented Mar 12, 2018

⚠️🚨DO NOT MERGE UNTIL pwa-buildpack#20 IS MERGED! 🚨⚠️

Removes the inline devtools directory and replaces it with imports from @magento/pwa-buildpack.

Changes webpack.config.js to use PWADevServer and ServiceWorkerPlugin.

@zetlen zetlen requested review from jimbo and DrewML March 12, 2018 14:50
@zetlen
Copy link
Contributor Author

zetlen commented Mar 12, 2018

The +10,000 is all in package-lock.json. :(

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

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

I think this is the right idea to simplify webpack configuration. Just a few minor changes to consider.

@@ -1,3 +1,5 @@
MOCK_IMAGES_PATH="https://magento-ux.github.io/pwaza"
MAGENTO_HOST="https://magento2.vagrant89"

MAGENTO_HOST="https://localhost:8443"
Copy link
Contributor

Choose a reason for hiding this comment

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

This was originally pointing to the vagrant box. Now it points to localhost. If I'm using vagrant, what should the value of MAGENTO_HOST be?

MAGENTO_HOST="https://magento2.vagrant89"

MAGENTO_HOST="https://localhost:8443"
MAGENTO_PUBLIC_PATH="/pub/static/frontend/Magento/venia/en_US/"
Copy link
Contributor

Choose a reason for hiding this comment

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

As of writing, this theme is still called rush, so I think venia won't work in this path.

@@ -32,9 +32,8 @@
"redux": "^3.7.2"
},
"devDependencies": {
"@magento/anhinga": "^0.2.0",
"@magento/pwa-buildpack": "^0.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Update this to @jzetlen/pwa-buildpack for now?

MagentoResolver,
PWADevServer
}
} = require('@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.

This certainly works, but nested destructuring is a bit difficult to read. Maybe we could change this to the following?

const { Webpack } = require(...);

const { ..., PWADevServer } = Webpack;

src: resolve(__dirname, 'src'),
assets: resolve(__dirname, 'web'),
output: resolve(__dirname, 'web/js')
};
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

new webpack.NoEmitOnErrorsPlugin(),
new webpack.EnvironmentPlugin({
NODE_ENV: isProd ? 'production' : 'development',
SERVICE_WORKER_FILE_NAME: magentoEnv.serviceWorkerFileName
SERVICE_WORKER_FILE_NAME: 'sw.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there an environment variable for this?

@ericerway ericerway removed this from the Milestone 2 milestone Apr 24, 2018
@DrewML
Copy link
Contributor

DrewML commented May 9, 2018

@zetlen I think a good chunk of these changes have made their way into the repo. Is there still work in here needing to get merged?

@zetlen
Copy link
Contributor Author

zetlen commented May 11, 2018

@DrewML Nope! This branch is no longer necessary. Thanks for looking over it.

@zetlen zetlen closed this May 11, 2018
@zetlen zetlen deleted the zetlen/use-buildpack-webpack branch May 11, 2018 19:03
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.

4 participants