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

Remove addDependencyTo from parcel-bundler #2166

Closed
wants to merge 3 commits into from
Closed

Remove addDependencyTo from parcel-bundler #2166

wants to merge 3 commits into from

Conversation

LandonSchropp
Copy link

↪️ Pull Request

Hi there! This issue was bugging me, so I thought I'd take a quick stab at fixing it. I'm not familiar with the Parcel codebase, and I might have missed something, so please let me know if this isn't the right way to go about this, or you're not interested in this fix at this time.

This fixes a persistent warning message when using posthtml-include. It's been about 10 months since posthtml-loader version 1.0.1 was released, and the warning makes it difficult to use Parcel.

Fixes #1920

💻 Examples

Here's an example of my project's build after adding and removing whitespace three times:

screen shot 2018-10-17 at 8 44 36 pm

🚨 Test instructions

The easiest way to test is to add an <include name="template"></include> to an HTML file.

✔️ 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

@@ -38,15 +38,7 @@ async function getConfig(asset) {
config = config || {};
const plugins = config.plugins;
if (typeof plugins === 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Without depConfig, this conditional is redundant and should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Should I drop the condition and call Object.keys(plugins).forEach(p => plugins[p]); directly?

Copy link
Author

Choose a reason for hiding this comment

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

I suppose it would make sense to assume plugins is always an object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I... call Object.keys(plugins).forEach(p => plugins[p]) directly?

To do what? Without the Object.assign, it's not doing anything (apart from burning cycles).

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the delayed response. Just to clarify, I should pull out the condition and the iteration of the plugins completely, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I've pushed up an update. Thanks for the quick turnaround!

This fixes a persistent warning message when using `posthtml-include`.
It's been about 10 months since `posthtml-loader` version `1.0.1` was
released, and the warning makes it difficult to use Parcel.

Fixes #1920
This was based on @chocolateboy's feedback in my PR.
@LandonSchropp
Copy link
Author

@chocolateboy I wanted to give a gentle bump on this. Have you had a chance to look at the updates?

@chocolateboy
Copy link
Contributor

chocolateboy commented Oct 28, 2018

@LandonSchropp Yes, it looks fine. I can't vouch for the removed test, though, and GitHub doesn't appear to provide a way for me to resolve/remove my obsolete feedback.

@LandonSchropp
Copy link
Author

@chocolateboy Any advice on what we need to do to get this guy merged? Is there anything else I should change? Should we ask somebody else to review for that test?

@LandonSchropp
Copy link
Author

I'm closing this PR. It looks like it's not going to get merged.

@andrewsosa
Copy link

@chocolateboy Was there anything blocking this from being merged? Not seeing all these messages would be a huge QOL improvement.

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.

Deprecation warning when using posthtml
4 participants