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

Packages: Add notices package #9617

Merged
merged 3 commits into from
Oct 27, 2018
Merged

Packages: Add notices package #9617

merged 3 commits into from
Oct 27, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 4, 2018

Closes #6388

This pull request seeks to extract notices behaviors from the editor module to a new notices module, intended to serve largely the same purpose, but generalized to serve use-cases outside the editor. The API also largely remains the same, except with the addition of a new context option for specifying a simplified "area" value for notices grouping.

Planned to fork out into separate pull requests:

Open question:

  • With introduction of "context", should we consider most of the notices as we have today as part of an "editor" context, or does the idea of a global context represent what we're showing today in the editor below the heading? It'd be my idea that what we have today serves the "global" usage, but it is screen-specific, and we might want to consider "properly global" notices (things like notifications center), where our notices aren't quite as global.
  • What should be included here so far as componentry? The "Notice" component seems generic enough that it's sensible to remain in @wordpress/components. With this module effectively serving as an orchestrator, it seems most all of the state-bound components should live within.

Future enhancements:

  • Our existing notices should ideally not include React elements, and rather represent content as a plain string. Most of our existing use of elements could be better served by a formal "actions" option (e.g. "View Post" on the save success) which is more future-compatible to notices design revisions.
  • I'm curious to explore whether the new context concept could be leveraged to eliminate most of what's served by the internal state of withNotices.

@aduth aduth added npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress labels Sep 4, 2018
@@ -0,0 +1,34 @@
{
"name": "@wordpress/notices",
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

1.0.0-beta.0 please :)

],
"main": "build/index.js",
"dependencies": {
"@babel/runtime-corejs2": "7.0.0-beta.56",
Copy link
Member

Choose a reason for hiding this comment

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

Is should be replaced with:

"@babel/runtime": "^7.0.0",

since we are now using Babel 7 stable.

@aduth
Copy link
Member Author

aduth commented Sep 6, 2018

@gziolo @youknowriad I'm curious your opinion on:

I copied onSubKey higher-order reducer creator from core-data. I'm starting to wonder if we might want to consider distributing these as some form of data/reducer utilities, akin to as @wordpress/compose is for higher-order components. Maybe a @wordpress/compose-data or @wordpress/compose-reducer.

Another option I'm contemplating is repurposing @wordpress/compose to serve for any higher-order function composition, made evident by its compose import which would otherwise need to be duplicated redundantly into this new package if one were to be created. I'm not sure if it might cause some confusion however to have all these functions as part of the same package, partly for bloat and partly because it becomes unclear whether an import is to serve the purpose of creating a higher-order component or a higher-order reducer. Granted, there already is some confusion already in that some of what we export is technically a "higher-higher-order-function" (higher-order component creators).

@aduth
Copy link
Member Author

aduth commented Sep 6, 2018

I put together a rough concept for what I'd considered to be improvements to the test helpers to accommodate for the need to ensure stores for package are loaded (by ensuring all package src/index.js are imported for test). It had some negative cascading effects, so I opted instead for a more simplified approach here (basically import '../../' to trigger index imports).

For future reference, here's what I'd put together as the replacement for Jest's testRunner option:

const { dirname, resolve } = require( 'path' );
const DefaultRunner = require( 'jest-jasmine2' );
const pkgUp = require( 'pkg-up' );

function isPackageTestPath( rootDir, testPath ) {
	return testPath.startsWith( resolve( rootDir, 'packages' ) );
}

function getPackageEntryFile( testPath ) {
	const pkg = pkgUp.sync( testPath );
	if ( pkg ) {
		return resolve( dirname( pkg ), 'src/index.js' );
	}
}

module.exports = function( globalConfig, config, environment, runtime, testPath ) {
	const runner = DefaultRunner( globalConfig, config, environment, runtime, testPath );

	if ( isPackageTestPath( config.rootDir, testPath ) ) {
		const entry = getPackageEntryFile( testPath );
		if ( entry ) {
			runtime.requireModule( entry );
		}
	}

	return runner;
};

@aduth aduth force-pushed the add/notices-module branch from 3b1f547 to eb9a43e Compare September 6, 2018 20:25
@aduth
Copy link
Member Author

aduth commented Sep 6, 2018

Updates:

  • Passing tests
  • Resolved conflicts
  • Use correct Babel dependencies

Remaining:

  • Document package
  • Transition deprecated usage (maybe)

@aduth aduth force-pushed the add/notices-module branch 3 times, most recently from 21e0376 to 839c4ba Compare October 12, 2018 17:02
@aduth aduth removed the [Status] In Progress Tracking issues with work in progress label Oct 12, 2018
@aduth
Copy link
Member Author

aduth commented Oct 12, 2018

This is now "complete" and could do for a proper review.

@gziolo gziolo modified the milestones: 4.1, 4.2 Oct 12, 2018
@aduth aduth force-pushed the add/notices-module branch from 839c4ba to be2108e Compare October 22, 2018 13:00
@gziolo
Copy link
Member

gziolo commented Oct 22, 2018

I copied onSubKey higher-order reducer creator from core-data. I'm starting to wonder if we might want to consider distributing these as some form of data/reducer utilities, akin to as @wordpress/compose is for higher-order components. Maybe a @wordpress/compose-data or @wordpress/compose-reducer.

Another option I'm contemplating is repurposing @wordpress/compose to serve for any higher-order function composition, made evident by its compose import which would otherwise need to be duplicated redundantly into this new package if one were to be created. I'm not sure if it might cause some confusion however to have all these functions as part of the same package, partly for bloat and partly because it becomes unclear whether an import is to serve the purpose of creating a higher-order component or a higher-order reducer. Granted, there already is some confusion already in that some of what we export is technically a "higher-higher-order-function" (higher-order component creators).

I don't find it as a big problem that we duplicate 10 lines of code. We could land it as is. However if I would have to vote where to put this helper function, I think it makes more sense to make @wordpress/compose a general purpose component which offers all tools which help with composition. The only drawback I see is that this package depends on @wordpress/element.

I put together a rough concept for what I'd considered to be improvements to the test helpers to accommodate for the need to ensure stores for package are loaded (by ensuring all package src/index.js are imported for test). It had some negative cascading effects, so I opted instead for a more simplified approach here (basically import '../../' to trigger index imports).

It seems like a nice improvement to the current experience if you assume that those stores should be initialized. Personally, I think the ugliness of those imports raises the question whether our code doesn't depend too much on the internal state of stores. Maybe we should be more cautious when introducing such dependencies. Maybe we should rethink the way we mock and verify the usage of the data store. For instance, we could create mock helper utility which would allow us to easily operate on stores:

const noticesStore = mockStore( 'core/notices', {
    actions: {
        createSuccessNotice: jest.fn(),
    }
} );

expect( dispatch( 'core/notices' ).createSuccessNotice ).not.toHaveBeenCalled();

This way you wouldn't have to import @wordpress/notices package at all, it could operate on @wordpress/data directly.

Another question is how often we could offer a low-level pure function which operates on data fetched from the store which could be much easier unit tested.

@gziolo
Copy link
Member

gziolo commented Oct 22, 2018

I added e9e8938 since token-list is used only in test.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I think the only part left to review is all the changes in store effects. Otherwise it all looks good.


dispatch( 'core/notices' ).createNotice( status, content, options );

return { type: '__INERT__' };
Copy link
Member

Choose a reason for hiding this comment

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

I think I saw a different keyword in one of @youknowriad's PRs. Can we standardize it or it doesn't matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Realistically, it doesn't matter much, particularly for deprecations slated for removal, but I can plan to align for consistency's sake.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see an instance of DO_NOTHING from his pull requests, so I'm going to assume it's either not been merged or no longer relevant. If it's not been merged, then... I win if I get mine in first 😛

status,
content,
isDismissible,
spokenMessage,
Copy link
Member

@gziolo gziolo Oct 22, 2018

Choose a reason for hiding this comment

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

This code assumes that from now on spokenMessage is always equal to content - is it correct comparing to the previous behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so, in that previously we'd allow spokenMessage to be of type element, which is no longer supported (must be string).

Copy link
Member

@gziolo gziolo Oct 26, 2018

Choose a reason for hiding this comment

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

Yeah right. It makes sense to consolidate now.

*
* @type {Array}
*/
const DEFAULT_NOTICES = [];
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we also move to constants.js file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. I think the main value in constants.js are for variable constants used across files. This one is specific to selectors, however.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nit: Should we also move to constants.js file?

I think I'd rather keep it here, per my previous comment.

*
* @return {Function} Higher-order reducer.
*/
export const onSubKey = ( actionProperty ) => ( reducer ) => ( state = {}, action ) => {
Copy link
Member

Choose a reason for hiding this comment

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

My second thought is that it might be also a good idea to move it to data package next to other helpers like combineReducers as it has a very similar application. Eventually, we could extract those utilities to their own package. It's very unlikely that we will use it without reducers :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I like that, especially if even the main fear would be bloat for a size-conscious module developer, it'd be easily omitted by tree shaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I like that, especially if even the main fear would be bloat for a size-conscious module developer, it'd be easily omitted by tree shaking.

Let's keep this to a follow-up pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this one is big enough 👍

} );
} );

it( 'should return action with context', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Super nit: we can make this statement more explicit - ...action with *custom* context

Copy link
Member Author

Choose a reason for hiding this comment

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

Super nit: we can make this statement more explicit - ...action with *custom* context

Updated in rebased 009b8e916b3ae9fac32af65261713feab454097f.

case 'CREATE_NOTICE':
// Avoid duplicates on ID.
return [
...reject( state, { id: action.notice.id } ),
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, I was using filter( not( ... ) ) - it's a nice shortcut and reads much better 👍

*/
import { DEFAULT_CONTEXT } from './constants';

/**
Copy link
Member

Choose a reason for hiding this comment

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

I really like how you document code. Just wanted to emphasize it as I look at this PR 💯
How do you align @param definitions?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you align @param definitions?

I try to follow the guidelines in the Documentation Standard which in practice are to keep each fragment space-aligned (type, name, description) within each line-delimited grouping (param, return, etc).

Copy link
Member

Choose a reason for hiding this comment

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

I see, 80 characters per line is difficult to achieve but makes sense 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot the 80 character thing was part of the standard. It's self-imposed elsewhere. It becomes ugly and tempting to bypass in cases like this, but I think it's a motivator to try to avoid complexity (e.g. in the naming of the type).

* @param {?Array<WPNoticeAction>} options.actions User actions to be
* presented with notice.
*/
export function* createNotice( status = 'info', content, options = {} ) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any docs about using generators with action creators? I would love to read it myself to better understand what is possible as of today. I feel like I'm missing some great capabilities by not knowing what options are provided out of the box.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have any docs about using generators with action creators?

At the moment, I think they're best described by:

Copy link
Member

@gziolo gziolo Oct 26, 2018

Choose a reason for hiding this comment

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

Thanks for sharing thos links, I will check them on Monday.

},
type: 'CREATE_NOTICE',
} ) );
expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalled();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why we can assert using more detailed check similar to what was previousle and what I see later in the file, e.g.:

expect( dataDispatch( 'core/notices' ).createErrorNotice ).toHaveBeenCalledWith( 'Publishing failed', { id: SAVE_POST_NOTICE_ID } );

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any reason why we can assert using more detailed check similar to what was previousle and what I see later in the file, e.g.:

expect( dataDispatch( 'core/notices' ).createErrorNotice ).toHaveBeenCalledWith( 'Publishing failed', { id: SAVE_POST_NOTICE_ID } );

Updated in rebased c5985282b2a2b977fb10ddb37eb8a797fa13e566.

@@ -37,6 +36,7 @@ import {
fetchReusableBlocks as fetchReusableBlocksAction,
} from '../../actions';
import reducer from '../../reducer';
import '../../..';
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment why we import this thing? It can be copy and paste from what we had before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we add a comment why we import this thing? It can be copy and paste from what we had before.

Updated in rebased 009b8e916b3ae9fac32af65261713feab454097f.

@@ -16,15 +16,17 @@ import {
cloneBlock,
} from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';
// TODO: Ideally this would be the only dispatch in scope. This requires either
Copy link
Member

Choose a reason for hiding this comment

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

I finally get the idea. It's not perfect but let's move on and refactor later.

@aduth @youknowriad how do you see the future or effects? Should we invest more time in improving them to make them capable of working with multiple stores?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we keep effects, I think we should probably opt for using dispatch and select APIs directly, in place of the second store argument. This requires refactoring to respect current registry context, which was raised recently in the context of selectors and controls as well.

Whether we want to keep effects: I go back and forth. I think many of what we have as effects currently would be better served by controls, though I don't know for certainty if there isn't a valid use-case for effects. One that I might have argued for previously as a valid "side effect" would have been the speak behaviors, but even as proposed here I've implemented that as a yielded control.

Copy link
Member

Choose a reason for hiding this comment

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

I personally really like how speak is implemented. It’s very easy to follow when you omit all the fancy syntax that it requires 😅 I asked that because if we bet on controls then we don’t have to worry about using global dispatch hrre. It’s just optical change when we pass it as argument rather than store.

@@ -131,13 +133,14 @@ export const saveReusableBlocks = async ( action, store ) => {
id,
} );
const message = isTemporary ? __( 'Block created.' ) : __( 'Block updated.' );
dispatch( createSuccessNotice( message, { id: REUSABLE_BLOCK_NOTICE_ID } ) );
dataDispatch( 'core/notices' ).createSuccessNotice( message, {
Copy link
Member

Choose a reason for hiding this comment

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

It makes me wonder if we should refactor effects to take data as param rather than store. This would align closer to what we have in other parts of the applications, in particular, withSelect and withDispatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes me wonder if we should refactor effects to take data as param rather than store.

By date, would you mean the current registry? I guess that would work as a solution for https://github.com/WordPress/gutenberg/pull/9617/files#r228623495, but as noted in that comment, we'll need a similar solution for selectors and controls as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is what I thought.

dispatch( removeNotice( SAVE_POST_NOTICE_ID ) );
dispatch( removeNotice( AUTOSAVE_POST_NOTICE_ID ) );
dataDispatch( 'core/notices' ).removeNotice( SAVE_POST_NOTICE_ID );
dataDispatch( 'core/notices' ).removeNotice( 'autosave-exists' );
Copy link
Member

Choose a reason for hiding this comment

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

Why do we stop using this AUTOSAVE_POST_NOTICE_ID constant? Are there any blockers? Can't we use the trick with SAVE_POST_NOTICE_ID?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kinda feel these constants are a waste of our resources, in the same way we don't bother with action type constants. I'll see about at least making things consistent, even if that means reintroducing the constant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we stop using this AUTOSAVE_POST_NOTICE_ID constant? Are there any blockers? Can't we use the trick with SAVE_POST_NOTICE_ID?

Looking at this more, I don't really like the idea of importing store constants into the EditorProvider component. And in general, I don't really like how we've implemented this notion of notices which should be dismissed on the next save. Rather than hard-coding their removal, it seems like it might be better to have some separate state tracking IDs of notices which should be removed on the next save. This way the EditorProvider could add to the state at the same time it's creating its notice, keeping the ID colocated.

Thoughts? Maybe better to defer to a follow-up pull request, keeping this one as-is?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we need to do decide on the future effects first. Then we can think about constants. As noted in another comment we probably shouldn’t use them at all in this context.

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 on mobile so I can’t check the last part of your comment. It sounds like a good idea. We can discuss in the future. Let’s get this in first 😃

@gziolo
Copy link
Member

gziolo commented Oct 23, 2018

There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!

Your own Travis check bites :)

I'm not sure why though. It doesn't change anything locally.

@gziolo
Copy link
Member

gziolo commented Oct 23, 2018

To answer more of your questions:

With introduction of "context", should we consider most of the notices as we have today as part of an "editor" context, or does the idea of a global context represent what we're showing today in the editor below the heading? It'd be my idea that what we have today serves the "global" usage, but it is screen-specific, and we might want to consider "properly global" notices (things like notifications center), where our notices aren't quite as global.

That's an interesting question. I would keep global for some time and see how it evolves. As far as I understand the place we render them is going to contain notices that as of today are considered global.

Our existing notices should ideally not include React elements, and rather represent content as a plain string. Most of our existing use of elements could be better served by a formal "actions" option (e.g. "View Post" on the save success) which is more future-compatible to notices design revisions.

Yes, that would be great as it would make it possible to persist those notices if we ever have to.

What should be included here so far as componentry? The "Notice" component seems generic enough that it's sensible to remain in @wordpress/components. With this module effectively serving as an orchestrator, it seems most all of the state-bound components should live within.

The only bits that could be converted into a component exposed in @wordpress/notices packages would be something similar to EditorNotices from @wordpress/editor package:

function EditorNotices( props ) {
	return (
		<NoticeList { ...props }>
			<TemplateValidationNotice />
		</NoticeList>
	);
}

export default compose( [
	withSelect( ( select ) => ( {
		notices: select( 'core/notices' ).getNotices(),
	} ) ),
	withDispatch( ( dispatch ) => ( {
		onRemove: dispatch( 'core/notices' ).removeNotice,
	} ) ),
] )( EditorNotices );

It would have to take context as one of the props, so this code could be rewritten as:

function EditorNotices() {
    return (
        <Notices>
            <TemplateValidationNotice />
        </Notices>
    );
}

The only challenge would be to pass the context to onRemove but it can be easily implemented by proxing dispatch method.

@gziolo
Copy link
Member

gziolo commented Oct 23, 2018

It works as expected with all the changes introduced. I can see all notices that were rendered before and I can use all links. I will give it 👍 as soon as Travis is happy and my questions are answered.

@youknowriad
Copy link
Contributor

Just a quick note that we should notify the people working on Core integration because this impacts the scripts registration...

'wp-rich-text',
'wp-url',
Copy link
Member

Choose a reason for hiding this comment

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

@atimmer @omarreiss @pento - pinging you, so you aware that this is coming up soon and we will have to update Core, too.

Copy link
Member

Choose a reason for hiding this comment

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

Just a quick note that we should notify the people working on Core integration because this impacts the scripts registration...

@youknowriad good call, done 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a new script to register...

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the notification, putting it on my list for when I update the packages based on Gutenberg 4.2.

expect( dispatch ).toHaveBeenCalledWith( expect.objectContaining( {
notice: {
content: <p>Post published.{ ' ' }<a>View post</a></p>, // eslint-disable-line jsx-a11y/anchor-is-valid
id: 'SAVE_POST_NOTICE_ID',
Copy link
Member Author

Choose a reason for hiding this comment

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

To my point about value of constants, we don't even use them consistently as constants.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let’s not use them at all then 😃

@aduth aduth force-pushed the add/notices-module branch from 3d4c7ee to 8e13dc1 Compare October 26, 2018 18:40
@aduth aduth force-pushed the add/notices-module branch from 8e13dc1 to c598528 Compare October 26, 2018 18:42
@aduth
Copy link
Member Author

aduth commented Oct 26, 2018

Thanks so much for the detailed review of this behemoth @gziolo .

I've pushed a rebased branch with conflicts resolved, feedback addressed, and commits squashed (with attribution preserved).

aduth and others added 2 commits October 26, 2018 16:00
Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>
Makes spokenMessage redundant by ensuring content is to be only assigned as plain string

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>
@aduth aduth force-pushed the add/notices-module branch from 6a61bf3 to bedb328 Compare October 26, 2018 20:00
@aduth aduth force-pushed the add/notices-module branch from bedb328 to bd1f614 Compare October 26, 2018 21:32
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback and your in-depth answers. Let’s 🚢 it as soon as Travis is happy about the lock file.

@aduth
Copy link
Member Author

aduth commented Oct 29, 2018

Created two issues to track follow-up work:

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

Successfully merging this pull request may close these issues.

4 participants