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

Attempt to fix #2340 #2372

Closed
wants to merge 2 commits into from
Closed

Conversation

emilniklas
Copy link

@emilniklas emilniklas commented Dec 7, 2018

↪️ Pull Request

Today, before the bundler creates a child/sibling bundle for a certain asset, it's checked that the asset isn't already an entry point for an existing bundle. However, if it is, the bundle isn't added as an actual child/sibling bundle. That leads to the issue described in #2340.

I think it should be okay to just add the already created bundles as child/sibling to the new bundle, but I'm not so well versed in this codebase, so I cannot way that for sure. I'm also quite unsure how to add a good test case for it (how can I make assertions on the output?).

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

Copy link
Member

@mischnic mischnic left a comment

Choose a reason for hiding this comment

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

I can't say much about the code changes themselves (there might be a performance/size reason for the deduplication), but regarding the test:

Take a look at the should insert sibling CSS bundles for JS files in the HEAD test:

it('should insert sibling CSS bundles for JS files in the HEAD', async function() {
let b = await bundle(
path.join(__dirname, '/integration/html-css/index.html')

That one already tests the CSS linking with only one HTML asset, you can just duplicate that test and modify it.

@devongovett
Copy link
Member

I'm a bit worried about changing the behavior of this at this point. This issue will definitely be fixed in Parcel 2.

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