Skip to content

Applies ignore rule before checking assets #286

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 16, 2021
Merged

Applies ignore rule before checking assets #286

merged 7 commits into from
Aug 16, 2021

Conversation

joaopslins
Copy link
Contributor

Fixes #280

fjsj
fjsj reviewed Jul 3, 2021
View reviewed changes
tests/app/settings.py Outdated
@@ -118,7 +118,10 @@
'CACHE': False,
'BUNDLE_DIR_NAME': 'bundles/',
'STATS_FILE': os.path.join(BASE_DIR, 'webpack-stats-app2.json'),
}
},
'NO_IGNORE': {
Copy link
Member

Choose a reason for hiding this comment

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

NO_IGNORE_APP is probably a better name, the NO_IGNORE -> IGNORE bugged my mind for few seconds 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fjsj
fjsj reviewed Jul 3, 2021
View reviewed changes
tests/app/tests/test_webpack.py Outdated
self.cleanup_bundles_folder()

def cleanup_bundles_folder(self):
rmtree('./assets/bundles', ignore_errors=True)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is really safe. I imagine some crazy situation of this test running in someones app (locally, but still).

Maybe the test setting should set a more custom name for the bundles dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fjsj
fjsj reviewed Jul 3, 2021
View reviewed changes
tests/app/tests/test_webpack.py Outdated
@@ -67,6 +72,18 @@ def test_simple_and_css_extract(self):
self.assertEqual(files['main.css']['path'], os.path.join(settings.BASE_DIR, 'assets/bundles/main.css'))
self.assertEqual(files['main.js']['path'], os.path.join(settings.BASE_DIR, 'assets/bundles/main.js'))

def test_default_ignore_config_ignores_map_files(self):
self.compile_bundles('webpack.config.sourcemaps.js')
chunks = get_loader('NO_IGNORE').get_bundle('main')
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this get_loader to use a string the the other to be imported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just that the other is the default config and it's being used in multiple tests. Since this one is specific for this case, then there is no need for it to be a const.

fjsj
fjsj reviewed Jul 3, 2021
View reviewed changes
webpack_loader/loader.py
@@ -39,15 +39,23 @@ def get_assets(self):
return self.load_assets()

def filter_chunks(self, chunks):
Copy link
Member

Choose a reason for hiding this comment

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

Did you check all calls to filter_chunks?

Copy link
Member

Choose a reason for hiding this comment

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

Also, could you please track this change of behavior in a Unreleased section of the CHANGELOG?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, there is only one call at

django-webpack-loader/webpack_loader/loader.py

Lines 92 to 102 in cc8c0a0

if assets.get('status') == 'done':
chunks = assets['chunks'].get(bundle_name, None)
if chunks is None:
raise WebpackBundleLookupError('Cannot resolve bundle {0}.'.format(bundle_name))
filtered_chunks = self.filter_chunks(chunks)
for chunk in filtered_chunks:
asset = assets['assets'][chunk]
if asset is None:
raise WebpackBundleLookupError('Cannot resolve asset {0}.'.format(chunk))

fjsj
fjsj reviewed Jul 3, 2021
View reviewed changes
webpack_loader/loader.py Outdated
assets = self.get_assets()

for chunk in chunks:
files = assets['assets']
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense for this line to be out of the for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fjsj
fjsj requested changes Jul 3, 2021
View reviewed changes
Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Looking good overall. Few comments.

@joaopslins joaopslins requested a review from rvlb August 16, 2021 18:22
@joaopslins joaopslins merged commit 729fbd8 into master Aug 16, 2021
@patrickdamery patrickdamery mentioned this pull request Aug 17, 2021
Closed
@joaopslins
Copy link
Contributor Author

Fixed in 1.3.0

@fjsj fjsj deleted the apply-ignore-before-checking-asset branch August 30, 2021 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjsj fjsj fjsj requested changes

@rvlb rvlb rvlb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build adding missing file name in chunk.app
3 participants
@joaopslins @fjsj @rvlb