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

Add custom LOADER_CLASS support #210

Merged
merged 1 commit into from Jan 13, 2020

Conversation

rhyneav
Copy link
Contributor

@rhyneav rhyneav commented Oct 29, 2019

This project has been a huge value add for us, and I'd like to help address some of the issues around adding custom loader class support. It sounds like the approach outlined in #83 gives the most flexibility for extending the loader. #202 looks like it's pretty close to that implementation, but given that it's been almost three months since opening (without any edits), and the change is relatively small, I'd like to open a new PR in hopes of getting these changes upstream.

Changes:
LOADER_CLASS on the WEBPACK_CONFIG setting is now where the loader class is defined. To retain backward compatibility and to keep getting started simple, this defaults to the existing WebpackLoader class.

WebpackLoader._load_assets has been renamed to WebpackLoader.load_assets. This keeps the API extendable when creating custom webpack loaders

Documentation has been updated to include how to extend the WebpackLoader using the loader class.

Tests have also been added.

I'm happy to implement any feedback or discuss the design choices! I have the bandwidth.

@noah-vetcove
Copy link

Would love to have this!

@maxlancaster
Copy link

This would also be huge for me too!

@owais owais added this to the 1.0.0 milestone Jan 8, 2020
@owais
Copy link
Collaborator

owais commented Jan 8, 2020

Thanks @rhyneav. Sorry for the delay. The changes look good to me. I think we can ship this in 0.7.0.

Can you please take a look at the CI failure and fix it if needed?

@rhyneav
Copy link
Contributor Author

rhyneav commented Jan 9, 2020

No worries at all @owais! I've been in your shoes before maintaining an open source project.

It looks like job 316.1 failed the test_request_blocking test case. This is the same one that is commented FIXME: This will work 99% time but there is no guarantee.... I double checked that this was passing on my machine. I think the CI just had an unlucky run. I'll force push this branch again to trigger another build.

Jobs 316.<7,8,9,10,11> failed due to InterpreterNotFound: python3.4. It looks like these tests are also failing on master. Could it be that something in Travis has changed? I don't see any changes in the tox.ini since the last successful run.

Please let me know if you need me to dig into anything else!

@owais
Copy link
Collaborator

owais commented Jan 10, 2020

Thanks. I'll try to fix the test suite over the weekend and hopefully we can merge this next week. PRs welcome as always :)

@rhyneav
Copy link
Contributor Author

rhyneav commented Jan 10, 2020

That sounds great! Thank you again for creating this project, it's been super helpful!

@brianwawok
Copy link

brianwawok commented Jan 11, 2020

Question from someone I think this might help:

In prod my files are in a CDN. If I made a custom stats file that had CDN urls instead of local files, will this work? Or so I need to do an extra step to modify the template piece as well?

Trying to upgrade from pipelines to this, and that is the last thing I gave to trace down.

@owais
Copy link
Collaborator

owais commented Jan 11, 2020

In prod my files are in a CDN. If I made a custom stats file that had CDN urls instead of local files, will this work?

Yes, this should work already without the need for this PR. We do support having a custom path in different environments.

@owais
Copy link
Collaborator

owais commented Jan 11, 2020

Check the publicPath config option for webpack-bundle-tracker: https://github.com/owais/webpack-bundle-tracker

@brianwawok
Copy link

brianwawok commented Jan 11, 2020 via email

@owais
Copy link
Collaborator

owais commented Jan 12, 2020

@rhyneav Migrated to CircleCI. Can you rebase this branch on master and push again please?

@rhyneav
Copy link
Contributor Author

rhyneav commented Jan 12, 2020

Nice! CircleCI is great. I just rebased and pushed back up. Please let me know if you'd like me to add anything else!

@owais
Copy link
Collaborator

owais commented Jan 12, 2020

@rhyneav I think CI was configured to not build PRs from forks. Can you please --amend the commit and force push to trigger new builds?

Thanks

`LOADER_CLASS` on the `WEBPACK_CONFIG` setting is now where the loader class
is defined. To retain backward compatibility and to keep getting started
simple, this defaults to the existing WebpackLoader class.

`WebpackLoader._load_assets` has been renamed to `WebpackLoader.load_assets`.
This keeps the API extendable when creating custom webpack loaders.

Documentation has been updated to include how to extend the WebpackLoader
using the `LOADER_CLASS`.
@rhyneav
Copy link
Contributor Author

rhyneav commented Jan 13, 2020

Just pushed!

@owais owais merged commit 152414c into django-webpack:master Jan 13, 2020
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.

None yet

5 participants