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
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
234e5b6
Add new warning package
diegohaz Dec 24, 2019
6649664
Add babel-plugin-dev-expression
diegohaz Dec 24, 2019
e3ba9d7
Remove babel-plugin-dev-expression
diegohaz Dec 26, 2019
a2732ec
Add babel-plugin to the warning pacakge
diegohaz Dec 26, 2019
93eddbb
Include @wordpress/warning/babel-plugin into @wordpress/babel-preset-…
diegohaz Dec 26, 2019
e357a17
Add warning to webpack.config.js
diegohaz Dec 26, 2019
ff25618
Merge branch 'master' into add/warning
diegohaz Jan 7, 2020
d42843e
Merge branch 'master' into add/warning
diegohaz Jan 9, 2020
be1f921
Check the presence of process.env
diegohaz Jan 9, 2020
0765740
Check presence of process.env in the code transformed by the babel pl…
diegohaz Jan 9, 2020
aa2d200
Move babel-plugin.js and its test file to the directory top-level
diegohaz Jan 9, 2020
b045842
Improve README with information about dead code removal
diegohaz Jan 9, 2020
47cce9f
Turn sideEffects into true in package.json
diegohaz Jan 9, 2020
bb8209a
Revert sideEffects change
diegohaz Jan 9, 2020
d046ef6
npm run docs:build
diegohaz Jan 9, 2020
25dfd7f
Update packages/warning/babel-plugin.js
diegohaz Jan 13, 2020
44cdafe
warning: Add types checking for warning package
aduth Jan 14, 2020
ac20da3
Merge branch 'master' into add/warning
diegohaz Jan 15, 2020
f0bf765
Avoid indentation on the warning function with an early return
diegohaz Jan 15, 2020
b389b34
Fix TS error on babel-plugin.js
diegohaz Jan 15, 2020
1724346
Accept a single string as message instead of multiple messages
diegohaz Jan 15, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bin/api-docs/packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const packages = [
'shortcode',
'url',
'viewport',
'warning',
'wordcount',
];

Expand Down
6 changes: 6 additions & 0 deletions docs/manifest-devhub.json
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,12 @@
"markdown_source": "../packages/viewport/README.md",
"parent": "packages"
},
{
"title": "@wordpress/warning",
"slug": "packages-warning",
"markdown_source": "../packages/warning/README.md",
"parent": "packages"
},
{
"title": "@wordpress/wordcount",
"slug": "packages-wordcount",
Expand Down
8 changes: 6 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
"@wordpress/token-list": "file:packages/token-list",
"@wordpress/url": "file:packages/url",
"@wordpress/viewport": "file:packages/viewport",
"@wordpress/warning": "file:packages/warning",
"@wordpress/wordcount": "file:packages/wordcount"
},
"devDependencies": {
Expand Down
1 change: 1 addition & 0 deletions packages/babel-preset-default/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ module.exports = function( api ) {
presets: [ getPresetEnv() ],
plugins: [
require.resolve( '@babel/plugin-proposal-object-rest-spread' ),
require.resolve( '@wordpress/warning/babel-plugin' ),
diegohaz marked this conversation as resolved.
Show resolved Hide resolved
[
require.resolve( '@wordpress/babel-plugin-import-jsx-pragma' ),
{
Expand Down
1 change: 1 addition & 0 deletions packages/babel-preset-default/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"@wordpress/babel-plugin-import-jsx-pragma": "file:../babel-plugin-import-jsx-pragma",
"@wordpress/browserslist-config": "file:../browserslist-config",
"@wordpress/element": "file:../element",
"@wordpress/warning": "file:../warning",
"core-js": "^3.1.4"
},
"publishConfig": {
Expand Down
1 change: 1 addition & 0 deletions packages/warning/.npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package-lock=false
Empty file added packages/warning/CHANGELOG.md
Empty file.
46 changes: 46 additions & 0 deletions packages/warning/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# Warning

Utility for warning messages to the console based on a passed condition.

## Installation

Install the module

```bash
npm install @wordpress/warning --save
```

_This package assumes that your code will run in an **ES2015+** environment. If you're using an environment that has limited or no support for ES2015+ such as lower versions of IE then using [core-js](https://github.com/zloirock/core-js) or [@babel/polyfill](https://babeljs.io/docs/en/next/babel-polyfill) will add support for these methods. Learn more about it in [Babel docs](https://babeljs.io/docs/en/next/caveats)._

## Reducing bundle size

Literal strings aren't minified. Keeping them in your production bundle may increase the bundle size significantly. To prevent that, you can put `@wordpress/warning/babel-plugin` into your [babel config](https://babeljs.io/docs/en/plugins#plugin-options) or use [`@wordpress/babel-preset-default`](https://www.npmjs.com/package/@wordpress/babel-preset-default), which already includes the babel plugin.

## API

<!-- START TOKEN(Autogenerated API docs) -->

<a name="default" href="#default">#</a> **default**

Shows a warning with `messages` if `condition` passes and environment is not `production`.

_Usage_

```js
import warning from '@wordpress/warning';

function MyComponent( props ) {
warning( ! props.title, '`props.title` was not passed' );
...
}
```

_Parameters_

- _condition_ `boolean`: Whether the warning will be triggered or not.
- _messages_ `Array<string>`: Message(s) to show in the warning.


<!-- END TOKEN(Autogenerated API docs) -->

<br/><br/><p align="center"><img src="https://s.w.org/style/images/codeispoetry.png?1" alt="Code is Poetry." /></p>
1 change: 1 addition & 0 deletions packages/warning/babel-plugin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require( './src/babel-plugin' );
27 changes: 27 additions & 0 deletions packages/warning/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"name": "@wordpress/warning",
"version": "0.0.1",
"description": "Warning utility for WordPress.",
"author": "The WordPress Contributors",
"license": "GPL-2.0-or-later",
"keywords": [
"wordpress",
"warning"
],
"homepage": "https://github.com/WordPress/gutenberg/tree/master/packages/warning/README.md",
"repository": {
"type": "git",
"url": "https://github.com/WordPress/gutenberg.git",
"directory": "packages/warning"
},
"bugs": {
"url": "https://github.com/WordPress/gutenberg/issues"
},
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": false,
"publishConfig": {
"access": "public"
}
}
62 changes: 62 additions & 0 deletions packages/warning/src/babel-plugin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
const pkg = require( '../package.json' );

function babelPlugin( { types: t } ) {
const seen = Symbol();

const binaryExpression = t.binaryExpression(
'!==',
t.memberExpression(
t.memberExpression( t.identifier( 'process' ), t.identifier( 'env' ), false ),
t.identifier( 'NODE_ENV' ),
false
),
t.stringLiteral( 'production' )
);

return {
visitor: {
ImportDeclaration( path, state ) {
const { node } = path;
const isThisPackageImport = node.source.value.indexOf( pkg.name ) !== -1;

if ( ! isThisPackageImport ) {
return;
}

const defaultSpecifier = node.specifiers.find(
( specifier ) => specifier.type === 'ImportDefaultSpecifier'
);

const { name } = defaultSpecifier.local;

state.callee = name;
},
CallExpression( path, state ) {
const { node } = path;

// Ignore if it's already been processed
if ( node[ seen ] ) {
return;
}

const name = state.callee || state.opts.callee;

if ( path.get( 'callee' ).isIdentifier( { name } ) ) {
// Turns this code:
// warning(condition, argument, argument);
// into this:
// process.env.NODE_ENV !== "production" ? warning(condition, argument, argument) : void 0;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

node[ seen ] = true;
path.replaceWith(
t.ifStatement(
binaryExpression,
t.blockStatement( [ t.expressionStatement( node ) ] )
)
);
}
},
},
};
}

module.exports = babelPlugin;
45 changes: 45 additions & 0 deletions packages/warning/src/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
function isDev() {
return (
typeof process !== 'undefined' &&
process.env &&
process.env.NODE_ENV !== 'production'
);
}

/**
* Shows a warning with `messages` if `condition` passes and environment is not `production`.
*
* @param {boolean} condition Whether the warning will be triggered or not.
* @param {string[]} messages Message(s) to show in the warning.
*
* @example
* ```js
* import warning from '@wordpress/warning';
*
* function MyComponent( props ) {
* warning( ! props.title, '`props.title` was not passed' );
* ...
* }
* ```
*/
export default function warning( condition, ...messages ) {
if ( isDev() ) {
diegohaz marked this conversation as resolved.
Show resolved Hide resolved
if ( ! condition ) {
return;
}

const text = messages.join( '\n' );
aduth marked this conversation as resolved.
Show resolved Hide resolved

// eslint-disable-next-line no-console
console.warn( text );

// Throwing an error and catching it immediately to improve debugging
// A consumer can use 'pause on caught exceptions'
// https://github.com/facebook/react/issues/4216
try {
throw Error( text );
} catch ( x ) {
// do nothing
}
}
}
102 changes: 102 additions & 0 deletions packages/warning/src/test/babel-plugin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/**
* External dependencies
*/
import { transform } from '@babel/core';

/**
* Internal dependencies
*/
import babelPlugin from '../babel-plugin';

function join( ...strings ) {
return strings.join( '\n' );
}

function compare( input, output, options = {} ) {
const { code } = transform( input, {
configFile: false,
plugins: [ [ babelPlugin, options ] ],
} );
expect( code ).toEqual( output );
}

describe( 'babel-plugin', function() {
it( 'should replace warning calls with import declaration', () => {
compare(
join(
'import warning from "@wordpress/warning";',
'warning(true, "a", "b");'
),
join(
'import warning from "@wordpress/warning";',
'process.env.NODE_ENV !== "production" ? warning(true, "a", "b") : void 0;'
)
);
} );

it( 'should not replace warning calls without import declaration', () => {
compare(
'warning(true, "a", "b");',
'warning(true, "a", "b");'
);
} );

it( 'should replace warning calls without import declaration with plugin options', () => {
compare(
'warning(true, "a", "b");',
'process.env.NODE_ENV !== "production" ? warning(true, "a", "b") : void 0;',
{ callee: 'warning' }
);
} );

it( 'should replace multiple warning calls', () => {
compare(
join(
'import warning from "@wordpress/warning";',
'warning(true, "a", "b");',
'warning(false, "b", "a");',
'warning(cond, "c");',
),
join(
'import warning from "@wordpress/warning";',
'process.env.NODE_ENV !== "production" ? warning(true, "a", "b") : void 0;',
'process.env.NODE_ENV !== "production" ? warning(false, "b", "a") : void 0;',
'process.env.NODE_ENV !== "production" ? warning(cond, "c") : void 0;'
)
);
} );

it( 'should identify warning callee name', () => {
compare(
join(
'import warn from "@wordpress/warning";',
'warn(true, "a", "b");',
'warn(false, "b", "a");',
'warn(cond, "c");',
),
join(
'import warn from "@wordpress/warning";',
'process.env.NODE_ENV !== "production" ? warn(true, "a", "b") : void 0;',
'process.env.NODE_ENV !== "production" ? warn(false, "b", "a") : void 0;',
'process.env.NODE_ENV !== "production" ? warn(cond, "c") : void 0;'
)
);
} );

it( 'should identify warning callee name by ', () => {
compare(
join(
'import warn from "@wordpress/warning";',
'warn(true, "a", "b");',
'warn(false, "b", "a");',
'warn(cond, "c");',
),
join(
'import warn from "@wordpress/warning";',
'process.env.NODE_ENV !== "production" ? warn(true, "a", "b") : void 0;',
'process.env.NODE_ENV !== "production" ? warn(false, "b", "a") : void 0;',
'process.env.NODE_ENV !== "production" ? warn(cond, "c") : void 0;'
)
);
} );
} );
Loading