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

Extensions: update webpack configuration to prevent plugin conflicts #17312

Merged
merged 2 commits into from
Jun 11, 2021

Conversation

creativecoder
Copy link
Contributor

@creativecoder creativecoder commented Sep 29, 2020

Fixes #17289

Changes proposed in this Pull Request:

  • Adds a custom setting for output.jsonpFunction to the webpack configuration for building extensions
  • This prevents conflicts in the default webpackJsonp JavaScript global when other WordPress plugins also load JavaScript built with webpack

See https://v4.webpack.js.org/configuration/output/#outputjsonpfunction for more information about the webpack configuration.

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

I've only been able to replicate #17289 with the release build of Jetpack, not in development environments, so I'm not sure how to test that this definitely fixes the issue.

But more generally, you can test by

  1. Build Jetpack extensions with jetpack build plugin and select jetpack.
  2. Load a post or page in the editor with your local Jetpack development site. Look at the output JS for extensions (such as editor-beta.js). You should see window["webpackJsonpJetpack"] in the built JS, rather than window["webpackJsonp"]

Proposed changelog entry for your changes:

  • Updates the webpack configuration for jsonpFunction when building JavaScript for extensions.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello creativecoder! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D50391-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@creativecoder creativecoder marked this pull request as ready for review September 29, 2020 22:15
@jetpackbot
Copy link

jetpackbot commented Sep 29, 2020

Scheduled Jetpack release: November 10, 2020.
Scheduled code freeze: November 3, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17312

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against fc5a9ae

@jeherve jeherve added the [Extension] External Media Extend all block editor media tools to support external providers label Sep 30, 2020
@creativecoder
Copy link
Contributor Author

Related PR: Automattic/wp-calypso#46252

@creativecoder creativecoder changed the title External Media: Remove lodash dependency to fix js error Extensions: update webpack configuration to prevent plugin conflicts Oct 26, 2020
@creativecoder creativecoder requested a review from a team October 26, 2020 03:30
@creativecoder creativecoder added [Type] Infrastructure [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Oct 26, 2020
@creativecoder
Copy link
Contributor Author

Note that Automattic/wp-calypso#46252 is merged, but the @automattic/calypso-build package still needs a release for the changes to be available through npm.

I've updated the description and marked this ready for review.

@creativecoder
Copy link
Contributor Author

PR for the update to @automattic/calypso-build

@jeherve jeherve added Build [Status] Blocked / Hold and removed [Type] Infrastructure [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 26, 2020
@jeherve
Copy link
Member

jeherve commented Oct 26, 2020

Noting that we won't be able to update calypso-build just yet, even when a 6.4.0 becomes available. We'll need to fix the issues that prevent us from updating to 6.3.0 first:
#16845

@creativecoder
Copy link
Contributor Author

Noting that we won't be able to update calypso-build just yet

Ugh.. okay good to know. I'm wondering if we should prioritize that sooner rather than later given the severity of the related bug (#17289)

@jeherve
Copy link
Member

jeherve commented Nov 4, 2020

yarn link the @automattic/calypso-build package from wp-calypso package you have locally to a local Jetpack development environment

How did you get this to work? I get the following message when I give this a try:

yarn link @automattic/calypso-build
yarn link v1.22.10
error No registered package found called "@automattic/calypso-build".

@creativecoder
Copy link
Contributor Author

How did you get this to work?

@jeherve my process was

From packages/calypso-build directory in wp-calypso repo, run yarn link (note that you may first need to yarn unlink if you've done this previously). You should see a message

success Registered "@automattic/calypso-build".
info You can now run `yarn link "@automattic/calypso-build"` in the projects where you want to use this package and it will be used instead.

From jetpack repo root, run yarn link "@automattic/calypso-build". You should see

success Using linked package for "@automattic/calypso-build".

@jeherve
Copy link
Member

jeherve commented Jan 14, 2021

Noting that this will also be fixed in Core, as per this PR:
WordPress/gutenberg#27985

@creativecoder
Copy link
Contributor Author

Noting that this PR is still blocked by required upgrade of calypso-build. Most recent PR for that upgrade is #19374.

@jeherve
Copy link
Member

jeherve commented Jun 10, 2021

Noting that this PR is still blocked by required upgrade of calypso-build. Most recent PR for that upgrade is #19374.

The update is now complete. 🎉

@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jun 10, 2021
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Do you think you could rebase, now that the Calypso build update is merged?

@creativecoder creativecoder force-pushed the fix/activecampaign-js-error branch from fc5a9ae to fd30f8c Compare June 10, 2021 16:14
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Jun 10, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 10, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: July 6, 2021.
  • Scheduled code freeze: June 28, 2021.

@creativecoder creativecoder added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 10, 2021
@creativecoder
Copy link
Contributor Author

This should now be ready to go

@creativecoder creativecoder requested a review from jeherve June 10, 2021 17:19
@sdixon194 sdixon194 added this to the jetpack/9.9 milestone Jun 10, 2021
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

Seems ok to me.

@sdixon194 sdixon194 dismissed jeherve’s stale review June 11, 2021 14:56

PR was rebased.

@sdixon194 sdixon194 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 11, 2021
@creativecoder creativecoder merged commit 940152f into master Jun 11, 2021
@creativecoder creativecoder deleted the fix/activecampaign-js-error branch June 11, 2021 15:10
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 11, 2021
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D50391-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@creativecoder
Copy link
Contributor Author

creativecoder commented Jun 11, 2021

r227168-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build [Extension] External Media Extend all block editor media tools to support external providers [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extensions: Jetpack blocks disappear when ActiveCampaign plugin is activated
6 participants