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

Prevent re-bundling WordPress packages #1781

Merged
merged 6 commits into from
Jan 3, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 27, 2018

  • Eliminate re-bundling of @wordpress/dom-ready and @wordpress/i18n to instead rely on existing bundles if available. Ensure both are marked as devDependencies.
  • Include new standalone bundles of @wordpress/dom-ready and @wordpress/i18n if not available (e.g. in WordPress 4.9 without Gutenberg).
  • Eliminate use of JSHint to avoid having to maintain duplicate duplicate linter as ESLint.

The @wordpress/dom-ready package is bundled with both WordPress 5.0 and Gutenberg as wp-dom-ready, so we should use it.

Also, @wordpress/i18n is not even used in the project anymore, so we don't need it at all.

Problem: If on 4.9 and Gutenberg is not installed, then we need to polyfill wp-dom-ready with jQuery.ready(). Nevertheless, we are currently depending on wp-i18n even though it may not be available either. So we might need to make the plugin require WordPress 5.0 or WordPress 4.9 with Gutenberg as the minimum version. Or perhaps in that case we should still bundle those dependencies as separate scripts that we register? If we want to support 4.9 without Gutenberg, then I suppose this is what we'd have to do. @felixarntz thoughts?

Closes #1763.

@googlebot googlebot added the cla: yes Signed the Google CLA label Dec 27, 2018
@felixarntz
Copy link
Collaborator

@westonruter Couldn't we bundle the wp-i18n and wp-dom-ready scripts in compiled versions and register them if they aren't yet available (by core)? I think that's the least error-prone solution, doesn't require too many changes. Whenever we decide to drop 4.x support, we can remove them without further change.

@westonruter westonruter changed the title [WIP] Remove redundant WordPress NPM packages Remove redundant WordPress NPM packages Dec 31, 2018
@westonruter westonruter changed the title Remove redundant WordPress NPM packages Prevent re-bundling WordPress packages Dec 31, 2018
@westonruter
Copy link
Member Author

@felixarntz this is ready to go!

@westonruter westonruter added this to the v 1.0.2 milestone Dec 31, 2018
@westonruter westonruter force-pushed the remove/redundant-dependencies-1.0 branch from 70f4400 to d5dbf32 Compare December 31, 2018 21:31
Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Two minor questions, otherwise good to go.

sourcesPointer();
} );
// Run at DOM ready.
jQuery( sourcesPointer );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might miss something, but shouldn't this use wp.domReady somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use wp.domReady but this script is already depending on jQuery so we can use jQuery ready instead which does the same thing.

includes/amp-helper-functions.php Outdated Show resolved Hide resolved
@westonruter westonruter force-pushed the remove/redundant-dependencies-1.0 branch from 8b6c0be to 8e2090e Compare January 2, 2019 19:23
@westonruter
Copy link
Member Author

Ok, issues have been addressed.

@westonruter westonruter merged commit c96e3cd into 1.0 Jan 3, 2019
@westonruter westonruter deleted the remove/redundant-dependencies-1.0 branch January 3, 2019 16:17
@westonruter westonruter mentioned this pull request Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants