Fix: Add client/extensions as root for server. #14731
Conversation
| @@ -93,7 +93,7 @@ const webpackConfig = { | |||
| }, | |||
| resolve: { | |||
| extensions: [ '', '.json', '.js', '.jsx' ], | |||
| root: [ path.join( __dirname, 'server' ), path.join( __dirname, 'client' ), __dirname ], | |||
There was a problem hiding this comment.
Better put __dirname at the start of this array instead of at the end. That was my fix, changing the entry path didn't even cross my mind :D
There was a problem hiding this comment.
Yeah, I guess even with the entry path, __dirname should probably still be first. I'll update it.
This adds `client/extensions` as a root path for the server build. This allows extension-based imports like is already allowed on the client. Where this makes a difference is especially in the reducers and sections, which are built by webpack loaders. This also specifies the root directory for the index.js entry point, because it was conflicting with `client/extensions/index.js`
coderkevin
force-pushed
the
fix/webpack-extensions-root
branch
from
bf99898 to
755a636
Compare
|
@blowery or @apeatling Can you take a look over this just to ensure it's the right thing to do and won't cause problems elsewhere? Thanks. |
coderkevin
added
[Status] Needs Review
|
How does this work with the test framework? We don't currently use the webpack module resolution stuff there. |
|
Shouldn't be extensions folder moved to the root folder to reflect the way it's going to be referenced in the code? Having folder and its subfolder as root might be confusing. |
That's a fair point, I think. Maybe it doesn't belong under For this conversation the question is: "Should we be able to import extension files by the extension's name as a context? (e.g. If the answer is "yes", then we need to merge this PR. Right now, we've got one foot in this and one foot out. This PR seeks to put the other foot in to match the first. It operates under the assumption that this is the way we want it because there's already code to support it. If this is not the case, we need to decide this ASAP because code is already being written with this assumption in mind. |
coderkevin
added
[Status] Ready to Merge
and removed
[Status] Needs Review
|
It would be good to mention this change in the Readmes, specially on the |
So we really want to rename I think the changes look good here. My concern would be bloating the server bundle, as we add more extensions — is this a valid concern? I haven't looked too deeply yet. |
|
See #2319 :) |
Given that the server bundle is already pulling in all extension reducers, I think that would be a different and potentially larger discussion. For that point, I'd say as long as we're building extensions statically in the same bundle, it will always be an issue. |
This adds
client/extensionsas a root path for the server build. Thisallows extension-based imports like is already allowed on the client.
Where this makes a difference is especially in the reducers and
sections, which are built by webpack loaders.
This also specifies the root directory for the index.js entry point,
because it was conflicting with
client/extensions/index.jsTo Test:
state/reducer.jsfile and change an import from a relative path to an absolute one (e.g.client/extensions/woocommerce/state/reducer.js, change'./ui/reducer'to'woocommerce/state/ui/reducer')npm start. With this change, it should build. Without this change, it will not.