Skip to content

Added support for loading Stats file from URL #83

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

Closed
wants to merge 2 commits into from

Conversation

prashnts
Copy link

In production, it is generally favourable to have the stats file loaded
from some external URL (and not from filesystem). This is especially
useful for heroku where we no longer need to build the static assets
every time we deploy.

In this commit, I have added an implementation for specifying
STATS_FILE_URL in the configuration. Also added corresponding tests
with a mocked request. (It could use a few more test cases to handle the
exceptions though.)

-- Prashant

In production, it is generally favourable to have the stats file loaded
from some external URL (and not from filesystem). This is especially
useful for heroku where we no longer need to build the static assets
every time we deploy.

In this commit, I have added an implementation for specifying
`STATS_FILE_URL` in the configuration. Also added corresponding tests
with a mocked request. (It could use a few more test cases to handle the
exceptions though.)

-- Prashant
@coveralls
Copy link

coveralls commented Oct 26, 2016

Coverage Status

Coverage decreased (-2.5%) to 95.455% when pulling dbf8719 on prashnts:master into 7bea208 on owais:master.

Copy link
Collaborator

@owais owais left a comment

Choose a reason for hiding this comment

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

Hi @prashnts. I'd like to suggest a different approach. What if we let users use custom implementation of the WebpackLoader class? We can add a new setting to define the path of the loader class to use as a string like other django components do and then at run time, try to import this class and use it instead of the stock one. Then anyone can implement their own stats file loading logic including loading from a remote server. In addition to that, users will also be able to override other behavior that the library does not support out of the box.

We would need to following to make it work

  • A new setting. WEBPACK_LOADER_CLASS ?
  • Rename _load_assets method to load_assets
  • Update documentation
  • Add tests for custom loader implementation

@prashnts
Copy link
Author

prashnts commented Nov 9, 2016

Sounds great, @owais! I'll open a new PR with the mentioned changes in a
couple of days. I think this should be a major version bump, though.

RE Implementation, I think the following plan of action might be good:

  • Add a backends module.
  • Define BaseStatsLoader abstract class.
  • Provide FileSystemStatsLoader, extending BaseStatsLoader having same
    behaviour as current one.
  • Possibly ship with additional URLStatsLoader which will be what my PR
    does.

Let me know what you think.

-- Prashant

@owais
Copy link
Collaborator

owais commented Nov 9, 2016

I think we should just ship the regular loader but rename the private method (_load_assets) to public one (load_assets). So,

  • Rename _load_assets to load_assets
  • Add a new setting called, WEBPACK_LOADER_CLASS which will hold fully qualified name of a python class as a string
  • In webpack loader, change the code that initializes WebpackLoader class to make it import and initialize the class dynamically based on the value of WEBPACK_LOADER_CLASS setting.
  • May be add a django check to warn users against missing loader class defined in settings.
  • Update documentation

I don't think we need to ship BaseLoader and FileSystem loader at this point. We can discuss shipping a URL loader class after we merge this but I think most users should be happy to have their own in house implementation.

Thanks for the contribution!

@owais
Copy link
Collaborator

owais commented Dec 4, 2016

Closing this. Let's create a new PR when the proposed solution is ready.

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