Skip to content
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

BUG Fix missing dist images #1031

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

tractorcow
Copy link
Contributor

Fixes #953

Just copied these from src/images to dist/images fixes the issue.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Tested this locally and it fixes the problem. However, I don't think it fully addresses the problem.

Maybe this is a bad assumption on my part, but I've always assumed that I should be able to delete the client/dist and rebuild my library without consequences. If that's correct, then we should update the webpack config to auto copy files from the client/src/images directory to client/dist/images.

This is probably how this bug got introduced in the first place ... someone deleted their dist folder and didn't realised the images were gone.

@maxime-rainville
Copy link
Contributor

Just pushed an update to auto copy the client/src/images directory to client/dist/images on every build.

@@ -65,7 +65,7 @@ before_script:
- if [[ $BEHAT_TEST ]]; then (vendor/bin/serve --bootstrap-file vendor/silverstripe/cms/tests/behat/serve-bootstrap.php &> artifacts/serve.log &); fi

# Install NPM dependencies
- if [[ $NPM_TEST ]]; then nvm install && nvm use && npm install -g yarn && yarn install --network-concurrency 1 && (cd vendor/silverstripe/admin && yarn install --network-concurrency 1) && yarn run build; fi
- if [[ $NPM_TEST ]]; then rm -rf client/dist && nvm install && nvm use && npm install -g yarn && yarn install --network-concurrency 1 && (cd vendor/silverstripe/admin && yarn install --network-concurrency 1) && yarn run build; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

By completely deleting the client/dist directory before the build, we make sure the build completely rebuild the dist folder.

@@ -54,6 +54,7 @@
"@storybook/addons": "^3.4.11",
"@storybook/react": "^3.4.11",
"babel-jest": "^23.6.0",
"copy-webpack-plugin": "^4",
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest version of "copy-webpack-plugin" is 5, but it's not compatible with webpack 2.

@NightJar NightJar merged commit 04ff935 into silverstripe:1.4 Feb 4, 2020
@NightJar
Copy link
Contributor

NightJar commented Feb 4, 2020

Thanks fellows, nice work both of you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants