Better support for @wordpress/element Fragment JSX#34217
Conversation
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~816 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~564 bytes added 📈 [gzipped]) DetailsReact components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
ockham
left a comment
There was a problem hiding this comment.
Makes a ton of sense 🚢
Do we need a Changelog entry for this?
|
A bit of detail, you can test this by replacing an app entry point with something including a diff --git a/apps/o2-blocks/src/editor.js b/apps/o2-blocks/src/editor.js
index 1cbc25a62b..b1ddef73ad 100644
--- a/apps/o2-blocks/src/editor.js
+++ b/apps/o2-blocks/src/editor.js
@@ -1,7 +1,3 @@
-/**
- * Internal dependencies
- */
-import './category';
-import './editor-notes/editor';
-import './prev-next/editor';
-import './todo/editor';
+function MyComponent() {
+ return <>👋 Howdy!</>;
+}This results in a failed build on master Actually testing this with /***/ "./src/editor.js":
/*!***********************!*\
!*** ./src/editor.js ***!
\***********************/
/*! no exports provided */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
"use strict";
__webpack_require__.r(__webpack_exports__);
/* harmony import */ var _wordpress_element__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! @wordpress/element */ "@wordpress/element");
/* harmony import */ var _wordpress_element__WEBPACK_IMPORTED_MODULE_0___default = /*#__PURE__*/__webpack_require__.n(_wordpress_element__WEBPACK_IMPORTED_MODULE_0__);
function MyComponent() {
return Object(_wordpress_element__WEBPACK_IMPORTED_MODULE_0__["createElement"])(_wordpress_element__WEBPACK_IMPORTED_MODULE_0__["Fragment"], null, "\uD83D\uDC4B Howdy!");
}
/***/ }),
/***/ "@wordpress/element":
/*!*****************************!*\
!*** external "wp.element" ***!
\*****************************/
/*! no static exports found */
/***/ (function(module, exports) {
module.exports = wp.element;
/***/ })Squint and you can see that Full output(function(e, a) { for(var i in a) e[i] = a[i]; }(window, /******/ (function(modules) { // webpackBootstrap
/******/ // The module cache
/******/ var installedModules = {};
/******/
/******/ // The require function
/******/ function __webpack_require__(moduleId) {
/******/
/******/ // Check if module is in cache
/******/ if(installedModules[moduleId]) {
/******/ return installedModules[moduleId].exports;
/******/ }
/******/ // Create a new module (and put it into the cache)
/******/ var module = installedModules[moduleId] = {
/******/ i: moduleId,
/******/ l: false,
/******/ exports: {}
/******/ };
/******/
/******/ // Execute the module function
/******/ modules[moduleId].call(module.exports, module, module.exports, __webpack_require__);
/******/
/******/ // Flag the module as loaded
/******/ module.l = true;
/******/
/******/ // Return the exports of the module
/******/ return module.exports;
/******/ }
/******/
/******/
/******/ // expose the modules object (__webpack_modules__)
/******/ __webpack_require__.m = modules;
/******/
/******/ // expose the module cache
/******/ __webpack_require__.c = installedModules;
/******/
/******/ // define getter function for harmony exports
/******/ __webpack_require__.d = function(exports, name, getter) {
/******/ if(!__webpack_require__.o(exports, name)) {
/******/ Object.defineProperty(exports, name, { enumerable: true, get: getter });
/******/ }
/******/ };
/******/
/******/ // define __esModule on exports
/******/ __webpack_require__.r = function(exports) {
/******/ if(typeof Symbol !== 'undefined' && Symbol.toStringTag) {
/******/ Object.defineProperty(exports, Symbol.toStringTag, { value: 'Module' });
/******/ }
/******/ Object.defineProperty(exports, '__esModule', { value: true });
/******/ };
/******/
/******/ // create a fake namespace object
/******/ // mode & 1: value is a module id, require it
/******/ // mode & 2: merge all properties of value into the ns
/******/ // mode & 4: return value when already ns object
/******/ // mode & 8|1: behave like require
/******/ __webpack_require__.t = function(value, mode) {
/******/ if(mode & 1) value = __webpack_require__(value);
/******/ if(mode & 8) return value;
/******/ if((mode & 4) && typeof value === 'object' && value && value.__esModule) return value;
/******/ var ns = Object.create(null);
/******/ __webpack_require__.r(ns);
/******/ Object.defineProperty(ns, 'default', { enumerable: true, value: value });
/******/ if(mode & 2 && typeof value != 'string') for(var key in value) __webpack_require__.d(ns, key, function(key) { return value[key]; }.bind(null, key));
/******/ return ns;
/******/ };
/******/
/******/ // getDefaultExport function for compatibility with non-harmony modules
/******/ __webpack_require__.n = function(module) {
/******/ var getter = module && module.__esModule ?
/******/ function getDefault() { return module['default']; } :
/******/ function getModuleExports() { return module; };
/******/ __webpack_require__.d(getter, 'a', getter);
/******/ return getter;
/******/ };
/******/
/******/ // Object.prototype.hasOwnProperty.call
/******/ __webpack_require__.o = function(object, property) { return Object.prototype.hasOwnProperty.call(object, property); };
/******/
/******/ // __webpack_public_path__
/******/ __webpack_require__.p = "";
/******/
/******/
/******/ // Load entry module and return exports
/******/ return __webpack_require__(__webpack_require__.s = "./src/editor.js");
/******/ })
/************************************************************************/
/******/ ({
/***/ "./src/editor.js":
/*!***********************!*\
!*** ./src/editor.js ***!
\***********************/
/*! no exports provided */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
"use strict";
__webpack_require__.r(__webpack_exports__);
/* harmony import */ var _wordpress_element__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! @wordpress/element */ "@wordpress/element");
/* harmony import */ var _wordpress_element__WEBPACK_IMPORTED_MODULE_0___default = /*#__PURE__*/__webpack_require__.n(_wordpress_element__WEBPACK_IMPORTED_MODULE_0__);
function MyComponent() {
return Object(_wordpress_element__WEBPACK_IMPORTED_MODULE_0__["createElement"])(_wordpress_element__WEBPACK_IMPORTED_MODULE_0__["Fragment"], null, "\uD83D\uDC4B Howdy!");
}
/***/ }),
/***/ "@wordpress/element":
/*!*****************************!*\
!*** external "wp.element" ***!
\*****************************/
/*! no static exports found */
/***/ (function(module, exports) {
module.exports = wp.element;
/***/ })
/******/ })));
//# sourceMappingURL=editor.js.map |
|
Thanks 😬 Sorry for not testing with Tree-shaking seems to outsmart me, so I have to actually make webpack believe the file does something in order to repro on diff --git a/apps/o2-blocks/src/editor.js b/apps/o2-blocks/src/editor.js
index 1cbc25a62b..e5fe650697 100644
--- a/apps/o2-blocks/src/editor.js
+++ b/apps/o2-blocks/src/editor.js
@@ -1,7 +1,3 @@
-/**
- * Internal dependencies
- */
-import './category';
-import './editor-notes/editor';
-import './prev-next/editor';
-import './todo/editor';
+export default function MyComponent() {
+ return <>👋 Howdy!</>;
+}
|
ockham
left a comment
There was a problem hiding this comment.
Merge once e2e tests pass +1
|
There may be some valid failures on e2e tests I'll need to take a look at. |
d26aff0 to
d6f9efa
Compare
d6f9efa to
c174c1d
Compare
|
After a rebase and a fresh deps update, there aren't any issues with tests and I think we can move forward with this. I'd appreciate reviews, especially from @Automattic/cylon @Automattic/serenity and @Automattic/ajax who are using the build tool in the monorepo. |
|
There are indeed failing e2e tests when accessing the editor, apparently from A quick search suggests this is coming from unguarded This could easily come from some of the @WordPress package updates in shrinkwrap, I'll need to dig around and see if issues have been reported and find out exactly what's going on. |
def967c to
df706bc
Compare
|
I believe failures in the editor e2e tests after running We'll need to wait for that to be fixed and released before upgrading |
|
I'm not excited about the fix in df706bc - we seem to have
|
Ooh, rebuilding seems to have fixed them 🎉 |
7167f91 to
dd0e8f1
Compare
|
Rebased to fix conflict, but noting full e2e was passing: |
ockham
left a comment
There was a problem hiding this comment.
This appears to be working fine now! 
(I might've noticed that build time has gone up 😕 )
|
At dd0e8f1
|
|
@sirreal FWIW, I've stopped using the 'Needs e2e Testing' label to retrigger e2e tests if just one of them is failing. Instead, I rebuild manually from within CircleCI's admin dashboard. |
* Better support for @wordpress/element Fragment JSX See WordPress/gutenberg#15120 * Upgrade dependencies * Add CHANGELOG entry * Add DefinePlugin FORCE_REDUCED_MOTION to webpack configs
See WordPress/gutenberg#15120
Matches Gutenberg babel preset for JSX:
https://github.com/WordPress/gutenberg/blob/af6e5ecd8b398e08127ad1b3c784ce97899de100/packages/babel-preset-default/index.js#L58-L70
Provides better
calypso-buildsupport for<></>.Testing instructions
calypso-buildshould continue to work as before.Provided that
<></>syntax was basically unsupported by the previous setup, this should introduce few or no changes for consumers.It will allow
<Fragment></Fragment>to be replaced by<></>, leveraging the correct@wordpress/elementFragment.