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

Add packages webpack config WiP #118

Closed
wants to merge 6 commits into
base: 5.0
from

Conversation

Projects
None yet
4 participants
@atimmer
Member

atimmer commented Oct 9, 2018

No description provided.

@youknowriad

Thanks for starting this 👍

return mediaConfig;
return config;

This comment has been minimized.

@youknowriad

youknowriad Oct 9, 2018

Collaborator

Is this array of multiple configs something webpack supports by default? I wasn't aware of it :)
Ideally, media could be just another package but I can see how this may be needed for some time.

@youknowriad

youknowriad Oct 9, 2018

Collaborator

Is this array of multiple configs something webpack supports by default? I wasn't aware of it :)
Ideally, media could be just another package but I can see how this may be needed for some time.

This comment has been minimized.

@atimmer

atimmer Oct 9, 2018

Member

I completely agree, I chose this approach pragmatically because I didn't want to touch the media config if possible.

@atimmer

atimmer Oct 9, 2018

Member

I completely agree, I chose this approach pragmatically because I didn't want to touch the media config if possible.

Show outdated Hide outdated tools/webpack/packages.js Outdated
Show outdated Hide outdated tools/webpack/packages.js Outdated
moment: 'moment',
jquery: 'jQuery',
lodash: 'lodash',
'lodash-es': 'lodash',

This comment has been minimized.

@youknowriad

youknowriad Oct 9, 2018

Collaborator

These externals are mostly downloaded by Gutenberg at build time. For WordPress, we should just add them as npm dependencies and bundle them as scripts (like the WordPress packages). You can see how I did that for lodash in this patch https://core.trac.wordpress.org/ticket/44987

(expect jQuery and tinymce of course which are already there)

@youknowriad

youknowriad Oct 9, 2018

Collaborator

These externals are mostly downloaded by Gutenberg at build time. For WordPress, we should just add them as npm dependencies and bundle them as scripts (like the WordPress packages). You can see how I did that for lodash in this patch https://core.trac.wordpress.org/ticket/44987

(expect jQuery and tinymce of course which are already there)

Show outdated Hide outdated tools/webpack/packages.js Outdated
Show outdated Hide outdated tools/webpack/packages.js Outdated
}
return config;
};

This comment has been minimized.

@youknowriad

youknowriad Oct 9, 2018

Collaborator

I suppose we need also a babel config and a dedicated rule essentially to extract the i18n messages since the rest is already taken care of in the published packages.

@youknowriad

youknowriad Oct 9, 2018

Collaborator

I suppose we need also a babel config and a dedicated rule essentially to extract the i18n messages since the rest is already taken care of in the published packages.

This comment has been minimized.

@atimmer

atimmer Oct 9, 2018

Member

I will put extracting the i18n on my list as a follow-up task, I think we should get this in first though.

@atimmer

atimmer Oct 9, 2018

Member

I will put extracting the i18n on my list as a follow-up task, I think we should get this in first though.

This comment has been minimized.

@youknowriad

youknowriad Oct 9, 2018

Collaborator

I'm fine delaying it to a separate commit :)

@youknowriad

youknowriad Oct 9, 2018

Collaborator

I'm fine delaying it to a separate commit :)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 9, 2018

Collaborator

Should we add all these new scripts into the "script-loader.php". cc https://core.trac.wordpress.org/ticket/44987 for inspiration.

Collaborator

youknowriad commented Oct 9, 2018

Should we add all these new scripts into the "script-loader.php". cc https://core.trac.wordpress.org/ticket/44987 for inspiration.

Show outdated Hide outdated tools/webpack/packages.js Outdated
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 9, 2018

There is a lot of duplication between Webpack in both Gutenberg and what is proposed in core. I'm wondering if we should try to find what's common and expose as a npm package to simplify maintenance.

gziolo commented Oct 9, 2018

There is a lot of duplication between Webpack in both Gutenberg and what is proposed in core. I'm wondering if we should try to find what's common and expose as a npm package to simplify maintenance.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 9, 2018

For the record, we should land WordPress/gutenberg#10429 soon. I'm doing the last round of testing. This is the last blocker for publishing all Gutenberg codebase to npm. I will take care of it tomorrow as the first thing.

gziolo commented Oct 9, 2018

For the record, we should land WordPress/gutenberg#10429 soon. I'm doing the last round of testing. This is the last blocker for publishing all Gutenberg codebase to npm. I will take care of it tomorrow as the first thing.

Show outdated Hide outdated tools/webpack/vendors.js Outdated
Show outdated Hide outdated tools/webpack/vendors.js Outdated
Show outdated Hide outdated tools/webpack/packages.js Outdated
entry: gutenbergPackages.reduce( ( memo, packageName ) => {
const name = camelCaseDash( packageName );
memo[ name ] = join( baseDir, `node_modules/@wordpress/${ packageName }` );
return memo;

This comment has been minimized.

@gziolo

gziolo Oct 10, 2018

Actually, this is pretty interesting as we could reference node_modules/@wordpress/${ packageName } also from Gutenberg itself. Just noting in case we would like to have shared code to rule them all :)
For reference:
https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L134

@gziolo

gziolo Oct 10, 2018

Actually, this is pretty interesting as we could reference node_modules/@wordpress/${ packageName } also from Gutenberg itself. Just noting in case we would like to have shared code to rule them all :)
For reference:
https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L134

@youknowriad

youknowriad approved these changes Oct 10, 2018 edited

This is looking good to me. We can't effectively test this at the moment, until we have the script registration on the server which I'm fine delaying until another commit.

Also, yes, let's wait for the npm releases which should happen quickly. I'll keep you updated.

@@ -50,35 +50,41 @@
},
"dependencies": {
"@babel/polyfill": "^7.0.0",

This comment has been minimized.

@gziolo

gziolo Oct 11, 2018

We might need also

"@babel/runtime-corejs2": "7.0.0",

as a dependency. Now, I think about it, it probably should be dependency inside Gutenberg as it is referenced inside generated JavaScript.

@gziolo

gziolo Oct 11, 2018

We might need also

"@babel/runtime-corejs2": "7.0.0",

as a dependency. Now, I think about it, it probably should be dependency inside Gutenberg as it is referenced inside generated JavaScript.

@gziolo

gziolo approved these changes Oct 11, 2018

We need to test it with PHP part, but overall this looks great. I really like the way you handled mapping between all external libraries (including polyfills) - everything is very clean. Let's move on and iterate when PHP part is moved into core.

@@ -0,0 +1,33 @@
var path = require( 'path' ),

This comment has been minimized.

@gziolo

gziolo Oct 11, 2018

Super minor: we could replace var with const to improve aesthetics :)

@gziolo

gziolo Oct 11, 2018

Super minor: we could replace var with const to improve aesthetics :)

@@ -38,7 +42,56 @@
"grunt-webpack": "^3.0.2",
"ink-docstrap": "^1.3.0",
"matchdep": "~2.0.0",
"source-map-loader": "^0.2.4",
"uglify-js": "^3.4.9",

This comment has been minimized.

@ntwb

ntwb Oct 11, 2018

This may cause issues, a conflict issue with grunt-contrib-uglify was reverted a couple of days ago via https://core.trac.wordpress.org/changeset/43686/

@ntwb

ntwb Oct 11, 2018

This may cause issues, a conflict issue with grunt-contrib-uglify was reverted a couple of days ago via https://core.trac.wordpress.org/changeset/43686/

This comment has been minimized.

@atimmer

atimmer Oct 12, 2018

Member

It doesn't say why it was reverted. But it might not be a problem because node_modules can deal with different versions of the same package. At least for dev dependencies.

@atimmer

atimmer Oct 12, 2018

Member

It doesn't say why it was reverted. But it might not be a problem because node_modules can deal with different versions of the same package. At least for dev dependencies.

@atimmer atimmer closed this Oct 12, 2018

@atimmer atimmer deleted the add-packages-webpack-config branch Oct 12, 2018

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