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

WIP: Update wordpress monorepo #17072

Closed
wants to merge 4 commits into from
Closed

Conversation

brbrr
Copy link
Contributor

@brbrr brbrr commented Sep 4, 2020

Fixes #

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

  • Go to '..'

Proposed changelog entry for your changes:

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello brbrr! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D49093-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

@jetpackbot
Copy link

jetpackbot commented Sep 4, 2020

Fails
🚫

node failed.

Log

Error:  { Error: [BABEL] /home/travis/build/Automattic/jetpack/dangerfile.js: Cannot find module '@babel/compat-data/plugin-bugfixes'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Function._module2.default._load (/home/travis/build/Automattic/jetpack/node_modules/override-require/dist/overrideRequire.js:43:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/home/travis/build/Automattic/jetpack/node_modules/@automattic/calypso-build/node_modules/@babel/preset-env/lib/plugins-compat-data.js:10:46)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.customModuleHandler (/home/travis/build/Automattic/jetpack/node_modules/danger/distribution/runner/runners/inline.js:116:28)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12) code: 'MODULE_NOT_FOUND' }
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 3c18e96

@@ -139,6 +144,10 @@ module.exports = [
/^@wordpress\/i18n$/,
path.join( __dirname, './extensions/shared/i18n-to-php' )
),
new webpack.NormalModuleReplacementPlugin(
/^@wordpress\/element$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

I had one more idea.

Having NormalModuleReplacementPlugin replace imports from @wordpress/element has proven tricky, since our replacement -- element-to-php.js -- would need to include all export symbols from that package (with only createInterpolateElement replaced with the identity). Unlike the counterpart for @wordpress/i18n, @wordpress/element contains a plethora of export symbols, and the mix of wildcard re-exports that we've so far tried in this PR hasn't worked; and listing them all manually would be a maintenance nightmare, and very fragile.

But the NormalModuleReplacementPlugin should also work at a deeper level, so we might try to replace the ./create-interpolate-element import at package root level instead:

Suggested change
/^@wordpress\/element$/,
/^\.\/create-interpolate-element$/,

(if my RegEx-fu isn't mistaken)

However, this means that we'll also need to stop externalising @wordpress/element, and include it in our build instead.

cc/ @brbrr @anomiex @jeherve

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like that might actually work (without needing to touch the externalised stuff?) Pushed a commit to #16763 (as deps are more up-to-date there). Let's continue there.

@ockham
Copy link
Contributor

ockham commented Dec 1, 2020

(This PR also needs a rebase, to include e.g. #17935. Also, the package versions in here are probably outdated now -- it might make sense to carry them over from #16763.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants