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

Add warning package #19317

Merged
merged 21 commits into from Jan 15, 2020
Merged

Add warning package #19317

merged 21 commits into from Jan 15, 2020

Conversation

@diegohaz
Copy link
Member

diegohaz commented Dec 24, 2019

Description

The need of a warning utility was brought to light by @gziolo in several issues:

How has this been tested?

Unit tests

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
babel.config.js Outdated Show resolved Hide resolved
Copy link
Member

gziolo left a comment

This is great! Thank you for starting this PR 👍

There is this question whether we want to have this Babel plugin included or we create our own variation enabled in the preset we expose for 3rd party projects for developers convenience.

There is one more thing to consider, how to wire the package into WordPress globals. It uses the default export so on principle it should be whitelisted in Webpack config:

new LibraryExportDefaultPlugin( [
'api-fetch',
'deprecated',
'dom-ready',
'redux-routine',
'token-list',
'server-side-render',
'shortcode',
].map( camelCaseDash ) ),

Well, it’s a quite interesting case as we probably don’t want to promote it’s usage in production mode. It probably doesn’t harm to do it for consistency. What it does it changes how this default export gets exposed as global from wp.warning.default to wp.warning.

@gziolo gziolo requested review from youknowriad and aduth Dec 24, 2019
@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Dec 26, 2019

Thanks for the review @gziolo! I've replaced babel-plugin-dev-expression with a custom version that ignores random warning calls that aren't pointing to @wordpress/warning. This is exposed as @wordpress/warning/babel-plugin.

I've also included that in @wordpress/babel-preset-default and added the entry on webpack.config.js.

This is the first time I write a babel plugin. Any suggestion on how to write it better is welcome! :)

// Turns this code:
// warning(condition, argument, argument);
// into this:
// process.env.NODE_ENV !== "production" ? warning(condition, argument, argument) : void 0;
Comment on lines 45 to 48

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

I'm very glad you are thinking about dead code elimination with this pull request.

One concern I have about this specific implementation is that it makes a few assumptions about the runtime and build environments that I'm not sure we should be comfortable to make:

  1. Something else in the build will ensure that process.env.NODE_ENV global is assigned, or that the generated code is only able to run in Node.js (and not in a browser, where process.env does not exist)
  2. Something else in the build will perform dead code elimination on unreachable code (like Webpack + Uglify/Terser).

If either of these are not met, then either the code will not in-fact be removed, or at worst it can cause errors in the runtime environment (e.g. trying to access property NODE_ENV of an undefined process.env in a browser).

What I'd wonder is: Is there any reason we'd not want to just remove the call expression here and now based on what process.env evaluates to in the Babel transform? I guess I can see how we might not want that though if, for example, we want the warnings to be logged in projects which depend on our packages (where the published packages would be built in a "production" environment?).

It all looks very similar to Facebook's internal invariant utility. I guess this is intentional? How do they handle it? Or, since those are used in internal projects, are they fine to make those assumptions about the runtime/build environment?

Alternatively, we could do some combination of:

  • Document the build environment assumptions
  • Check that process global exists before evaluating it
    • e.g. typeof process !== 'undefined' && typeof process.env !== 'undefined' && process.env.NODE_ENV

This comment has been minimized.

Copy link
@diegohaz

diegohaz Jan 7, 2020

Author Member

@aduth I think I assumed we were handling process.env.NODE_ENV somehow because it's being used in other parts of the code base. For example:

if ( 'development' === process.env.NODE_ENV ) {
// eslint-disable-next-line no-console
console.error( 'Tooltip should be called with only a single child element.' );
}

Looking at the code generated in the build/build-module folders, that check remains there. I'm confused now! How does it work? Where can I find the code responsible for building the code for the browser?

What I'd wonder is: Is there any reason we'd not want to just remove the call expression here and now based on what process.env evaluates to in the Babel transform? I guess I can see how we might not want that though if, for example, we want the warnings to be logged in projects which depend on our packages (where the published packages would be built in a "production" environment?).

I expect some warnings like this:

warning(
  dialog.tabIndex === undefined || dialog.tabIndex < 0,
  'It\'s recommended to have at least one tabbable element inside dialog. The dialog element has been automatically focused.',
  'If this is the intended behavior, pass `tabIndex={0}` to the dialog element to disable this warning.'
);

If we remove the warning call from the code we ship to npm, then people won't see that (when using @wordpress/components, for example).

This comment has been minimized.

Copy link
@gziolo

gziolo Jan 8, 2020

Member

Facebook uses __DEV__:

https://github.com/facebook/react/blob/3c54df0914f02fed146faa519a5a899d0e3af32e/packages/shared/consoleWithStackDev.js#L31

I guess the benefit is that it fallbacks to undefined and therefore doesn't call the guarded code. You can still benefit from dead code elimination by teaching webpack to convert it to true or false.

I don't know what's the best way to move forward, but I'm sharing how I see the approach FB took.

This comment has been minimized.

Copy link
@aduth

aduth Jan 8, 2020

Member

I guess the benefit is that it fallbacks to undefined and therefore doesn't call the guarded code. You can still benefit from dead code elimination by teaching webpack to convert it to true or false.

In this case, I assume something must still be assigning __DEV__ value, as otherwise the reference to an undeclared variable would throw an error:

( function() {
    if ( __DEV__ ) {
        console.log( 'Constant assigned' );
    } else {
        console.log( 'Constant not assigned' );
    }
}() )
// Uncaught ReferenceError: __DEV__ is not defined

Revisiting my earlier comment, I'm actually not sure we're doing anything wrong here with how we reference process.env.NODE_ENV. I think it could be fair to say that if we're distributing this as a Node package, it may be more-or-less assumed that it be evaluated as in a Node context where the process global is defined. I do worry that in a standalone context, there is a chance that this might not be substituted correctly when building for a browser target. At least in the case of Webpack, this is handled automatically as far as I can tell (see mode, DefinePlugin).

A safe option might just be to test its presence before evaluating it (typeof process !== 'undefined').

With regards to other existing code in the codebase, I think one of two of the following might be the case:

  • We're okay to assume the code is not used outside of Gutenberg, or at least in a context not already subjected to Gutenberg's own build process
  • Or, that code is prone to issues as well

For something like a warning package, it seemed highly likely we'd want to promote that this be used separate from Gutenberg and its build, hence the original concern.

@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Jan 9, 2020

Added a check on the presence of process.env (be1f921)

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 9, 2020

Added a check on the presence of process.env (be1f921)

As far as I can tell, in the version published to npm, we still will see process.env.NODE_ENV check added from the Babel plugin.

For something like a warning package, it seemed highly likely we'd want to promote that this be used separate from Gutenberg and its build, hence the original concern.

Still, this should be fine with this proposal. If you don't use the Babel plugin, it should work in all cases.

Copy link
Member

aduth left a comment

@gziolo It's unclear to me from your comment: Are you expecting any other changes here?

It's generally looking quite good to me. My previous point still stands that we're expecting that someone has dead-code elimination as part of their build pipeline for removing the string literals. But we're not documenting this anywhere. In fact, we're documenting that the custom Babel plugin alone would be enough to eliminate the string literals. It might be something where it's enough to include some extra documentation that Uglify or Terser should be used in combination with the Babel plugin (and process.env.NODE_ENV substitution) in order for the strings to be removed from a production bundle.

packages/babel-preset-default/index.js Show resolved Hide resolved
"resolved": "http://registry.npmjs.org/clone-deep/-/clone-deep-0.2.4.tgz",
"resolved": "https://registry.npmjs.org/clone-deep/-/clone-deep-0.2.4.tgz",
Comment on lines 22853 to 22857

This comment has been minimized.

Copy link
@aduth

aduth Jan 9, 2020

Member

This value has a bad tendency to flip back and forth in local environments. It's not strictly part of your changes here, but I think I've seen the same locally, so it might be good to include this anyways, just to avoid future issues.

This comment has been minimized.

Copy link
@gziolo

gziolo Jan 13, 2020

Member

Rights, it always shows up for me locally as https. I even updated it in master a few days ago:
fe258a6

There might be something going on with operating systems or versions of npm client.

@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Jan 9, 2020

As far as I can tell, in the version published to npm, we still will see process.env.NODE_ENV check added from the Babel plugin.

Updated the babel plugin to check the presence of process.env in the transformed code as well (0765740).

My previous point still stands that we're expecting that someone has dead-code elimination as part of their build pipeline for removing the string literals. But we're not documenting this anywhere. In fact, we're documenting that the custom Babel plugin alone would be enough to eliminate the string literals. It might be something where it's enough to include some extra documentation that Uglify or Terser should be used in combination with the Babel plugin (and process.env.NODE_ENV substitution) in order for the strings to be removed from a production bundle.

Updated the README with more information about dead code elimination (b045842). It can be improved though.

diegohaz added 2 commits Jan 9, 2020
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 13, 2020

I tested it with the following changes:

diff --git a/package-lock.json b/package-lock.json
index 3c8c07c44..ce01de8a8 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -8251,7 +8251,8 @@
 			"version": "file:packages/a11y",
 			"requires": {
 				"@babel/runtime": "^7.4.4",
-				"@wordpress/dom-ready": "file:packages/dom-ready"
+				"@wordpress/dom-ready": "file:packages/dom-ready",
+				"@wordpress/warning": "file:packages/warning"
 			}
 		},
 		"@wordpress/annotations": {
diff --git a/packages/a11y/package.json b/packages/a11y/package.json
index eb2f11f3a..a28f44ad5 100644
--- a/packages/a11y/package.json
+++ b/packages/a11y/package.json
@@ -23,7 +23,8 @@
 	"react-native": "src/index",
 	"dependencies": {
 		"@babel/runtime": "^7.4.4",
-		"@wordpress/dom-ready": "file:../dom-ready"
+		"@wordpress/dom-ready": "file:../dom-ready",
+		"@wordpress/warning": "file:../warning"
 	},
 	"publishConfig": {
 		"access": "public"
diff --git a/packages/a11y/src/index.js b/packages/a11y/src/index.js
index cb79e767b..d7f0f76e5 100644
--- a/packages/a11y/src/index.js
+++ b/packages/a11y/src/index.js
@@ -2,6 +2,7 @@
  * WordPress dependencies
  */
 import domReady from '@wordpress/dom-ready';
+import warning from '@wordpress/warning';
 
 /**
  * Internal dependencies
@@ -14,6 +15,7 @@ import filterMessage from './filterMessage';
  * Create the live regions.
  */
 export const setup = function() {
+	warning( true, 'How is it going?' );
 	const containerPolite = document.getElementById( 'a11y-speak-polite' );
 	const containerAssertive = document.getElementById( 'a11y-speak-assertive' );

I can confirm that the warning is printed on the JS console only for the development build and I can't find any referenced to the warning package in the production mode.

Noting that, one of the side-effects of using this long safety checking for the process.env is that in development mode it gets polyfilled by webpack:

Screen Shot 2020-01-13 at 11 24 27

@gziolo
gziolo approved these changes Jan 13, 2020
Copy link
Member

gziolo left a comment

@gziolo It's unclear to me from your comment: Are you expecting any other changes here?

It's generally looking quite good to me. My previous point still stands that we're expecting that someone has dead-code elimination as part of their build pipeline for removing the string literals. But we're not documenting this anywhere. In fact, we're documenting that the custom Babel plugin alone would be enough to eliminate the string literals. It might be something where it's enough to include some extra documentation that Uglify or Terser should be used in combination with the Babel plugin (and process.env.NODE_ENV substitution) in order for the strings to be removed from a production bundle.

I wasn't really promoting any further code changes but I see that @diegohaz applied them regardless. In addition, the proposed solution works out of the box in all cases so now it feels that we could leave it as is if we don't care as much about having the process polyfill included in the development mode only.

@aduth - I'd defer the final call to you, from my perspective this patch is good to go. I'm fine with leaving it as is and it's also fine to remove the check from the Babel plugin in favor of the comment in the package description.

diegohaz and others added 2 commits Jan 13, 2020
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
@aduth
aduth approved these changes Jan 14, 2020
Copy link
Member

aduth left a comment

This is looking good to me.

The one thing I would propose to consider is the review comment concerning whether we want to make the variadic messages part of the public API.

I also pushed a commit 44cdafe to add this package to TypeScript types checking, to avoid follow-on work related to #18838. I hope you don't mind. This also includes type hints for the Babel plugin, which helped to verify there were no issues with the implementation there.

packages/warning/babel-plugin.js Show resolved Hide resolved
packages/warning/src/index.js Outdated Show resolved Hide resolved
packages/warning/src/index.js Outdated Show resolved Hide resolved
@diegohaz diegohaz merged commit 8175deb into master Jan 15, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@diegohaz diegohaz deleted the add/warning branch Jan 15, 2020
@aduth aduth mentioned this pull request Jan 15, 2020
9 of 69 tasks complete
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.