fix(dev): inherit local webpack.dev.config.js #74
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
regisb
approved these changes
Oct 31, 2022
Contributor
regisb
left a comment
regisb
left a comment
There was a problem hiding this comment.
I didn't test this change myself but it looks good to me!
Contributor
Author
|
@arbrandes would you mind having a quick look ? |
3 tasks
arbrandes
approved these changes
Nov 7, 2022
Collaborator
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
Approved, with a single question which you can tackle at your discretion.
|
|
||
| const baseDevConfig = require('@edx/frontend-build/config/webpack.dev.config.js'); | ||
| const baseDevConfig = ( | ||
| !fs.existsSync('./webpack.dev.config.js') |
Collaborator
There was a problem hiding this comment.
Nit: wouldn't an affirmation make the code a bit more readable, here? (As opposed to a negation.)
Contributor
Author
There was a problem hiding this comment.
From a performance viewpoint, most MFEs use the resolved frontend-platform config file (whereas the negation)
But I'd rather have a clearer code reading instead, So I've switched to the affirmative.
@arbrandes
| ENV PUBLIC_PATH='/{{ app["name"] }}/' | ||
| EXPOSE {{ app['port'] }} | ||
| CMD ["npm", "run", "start"] | ||
| CMD ["npm", "run", "start", "---", "--config", "./webpack.dev-tutor.config.js"] |
fixes issue overhangio#71 for to account for MFEs that have an existing webpack.dev.config.js
ARMBouhali
force-pushed
the
fix-overwriting-existing-webpack-dev-config
branch
from
886c78b to
bb150df
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A small fix for issue #71 to avoid overwriting the local
webpack.dev.config.jsin MFEs with the plugin's version of the file mounted at container startThe MFEs targetted by this all have a local
webpack.dev.config.js. Here are the ones branched for the olive release:To account for the 2 possible cases for all MFEs, the fix was tested and confirmed to work with:
frontend-app-gradebook(using local config)frontend-app-learning(using config resolved fromfrontend-platform)The modification in
Dockerfilewon't trigger a long image rebuild as it affects only the last step (CMD).My choice at the moment was to rename the plugin's
webpack.dev.config.jstowebpack.dev-tutor.config.js.Please tell me if another file name or location is preferable (e.g.
tutor/webpack.dev.config.js).