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: Move data module to the package maintained by Lerna #6828

Merged
merged 10 commits into from May 30, 2018

Conversation

@gziolo
Member

gziolo commented May 18, 2018

Description

Related issues: #3955, #6594.
Similar: #6658, #6756, #6758.

This PR tries to provide Lerna setup similar to what we have in WordPress/packages. This would allow publishing source code of individual modules to npm to make them available for all plugin developers.

After moving date and element, this PR moves data module to packages folder maintained by Lerna.

This is the first time where a package depends on another Gutenberg package (element).

Open question

data module uses window.localStorage which won't work in non-browser environments. Are we fine with having it included in the initial release? Should we add guard which disable persistence when local storage is not provided?

How has this been tested?

Manually:

  • Removed node_modules folder.
  • npm install
  • npm test
  • npm run dev
  • npm run build
  • npm run package-plugin

Types of changes

Refactoring.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@gziolo gziolo self-assigned this May 18, 2018

@gziolo gziolo requested review from ntwb and youknowriad May 18, 2018

@gziolo gziolo added this to the 3.0 milestone May 18, 2018

@gziolo gziolo requested a review from WordPress/gutenberg-core May 18, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 18, 2018

data module uses window.localStorage which won't work in non-browser environments. Are we fine with having it included in the initial release?

For me it's fine. I think it's not even used by default unless you explicitly import the persisting helper.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 18, 2018

This is the first time where a package depends on another Gutenberg package (element).

Since this is the first packages that users element I think we should discuss how we do JSX. At the moment we're relying on the wp.element.createElement pragma (global variable) but the @wordpress/element package don't generate the global by default and even if does it needs to be imported at least once to generate the global.

So what I propose is to do something similar to React:

  • require import { createElement } from '@wordpress/element' in all files using JSX
  • Update the eslint pragma config to match this change
@@ -1,9 +1,17 @@
# Data
# @wordpress/date

This comment has been minimized.

@travislopes

travislopes May 18, 2018

Contributor

Should be @wordpress/data

This comment has been minimized.

@gziolo

gziolo May 18, 2018

Member

Copy pasta 😃

@gziolo gziolo requested a review from mtias May 21, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented May 21, 2018

For me it's fine. I think it's not even used by default unless you explicitly import the persisting helper.

True, however, I added a way to override the default persistence storage with 04e52b0 :)

@gziolo

This comment has been minimized.

Member

gziolo commented May 21, 2018

So what I propose is to do something similar to React:

  • require import { createElement } from '@wordpress/element'` in all files using JSX
    Update the eslint pragma config to match this change

Sounds like a good idea. With the small exception, we should limit Eslint check to packages folder. We also need to make sure Eslint doesn't complain when createElement is included when used with JSX powered file.

@gziolo

This comment has been minimized.

Member

gziolo commented May 23, 2018

require import { createElement } from '@wordpress/element' in all files using JSX
Update the eslint pragma config to match this change

Done with 6c26c4b.

There is one more thing which popped up after rebase. We need to move deprecated util to its own package.

@gziolo

This comment has been minimized.

Member

gziolo commented May 23, 2018

There is one more thing which popped up after rebase. We need to move deprecated util to its own package.

Introduced also @wordpress/devtools package. We might want to find a better name for it. I'm happy to update it if you have better ideas.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 23, 2018

@gziolo Do you think it's possible to extract the devtools package to its own PR, I feel it could make it way quicker than the data one?

@gziolo

This comment has been minimized.

Member

gziolo commented May 23, 2018

@gziolo Do you think it's possible to extract the devtools package to its own PR, I feel it could make it way quicker than the data one?

Sure thing - it is in its own commit - doing it now.

@gziolo gziolo referenced this pull request May 23, 2018

Merged

Deprecated: Introduce new module with depreaction util #6914

4 of 4 tasks complete
@gziolo

This comment has been minimized.

Member

gziolo commented May 23, 2018

@youknowriad - I opened #6914 with devtools part, I will rebase this PR as soon as the other one lands.

@gziolo gziolo referenced this pull request May 24, 2018

Merged

Packages: Move element to packages maintained by Lerna #6756

3 of 3 tasks complete
@gziolo

This comment has been minimized.

Member

gziolo commented May 28, 2018

deprecated packages is now merged into master and this PR was rebased to use it.

},
},
},
],

This comment has been minimized.

@youknowriad

youknowriad May 28, 2018

Contributor

I believe we still need a similar config for babel to generate the Right JS (instead of using the global)

This comment has been minimized.

@gziolo

gziolo May 28, 2018

Member

Confirmed, I will look into it.

This comment has been minimized.

@gziolo

gziolo May 28, 2018

Member

It should be fixed with f7afc46.

@gziolo gziolo referenced this pull request May 28, 2018

Merged

Blob: Extract new `blob` package out of utils module #6973

3 of 3 tasks complete
@youknowriad

LGTM 👍 I'd appreciate a confirmation from @mtias or @aduth That the explicit import.

@Shelob9 Shelob9 referenced this pull request May 29, 2018

Closed

Packages: Publish all modules as independent npm packages #3955

19 of 19 tasks complete

@youknowriad youknowriad modified the milestones: 3.0, 3.1 May 29, 2018

@aduth

I'm fine with explicit createElement pragma. Would hope we'd apply it consistently eventually.

@@ -36,6 +36,8 @@ const DONE = chalk.reset.inverse.bold.green( ' DONE ' );
*/
const babelDefaultConfig = require( '@wordpress/babel-preset-default' );
babelDefaultConfig.babelrc = false;
// TODO: It should become the default value when all modules are moved to packages.
babelDefaultConfig.plugins[ 1 ][ 1 ].pragma = 'createElement';

This comment has been minimized.

@aduth

aduth May 29, 2018

Member
  1. Must we mutate? Can we assign into a new object? Not sure what would break, but certainly seems prone to cause breakage as implemented.
  2. Similarly, the [ 1 ][ 1 ] assumes a very specific order which, while we maintain and is unlikely to change anytime soon, is certainly incredibly fragile.

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

I opted for a quick fix with the hope that it won't last for long. I guess it is going to be around for a couple of weeks at least so I'm happy to provide more solid processing of default Babel config.

This comment has been minimized.

@gziolo
Install the module
```bash
npm install @wordpress/data@next --save

This comment has been minimized.

@aduth

aduth May 29, 2018

Member

Why do we @next ?

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

You are right, we can publish alpha without using dev :)

"url": "https://github.com/WordPress/gutenberg/issues"
},
"main": "src/index.js",
"dependencies": {

This comment has been minimized.

@aduth

aduth May 29, 2018

Member

Tests use deep-freeze and enzyme. Do we need to reflect that anywhere in this file as a dependency?

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Given that we are going to move those packages elsewhere one day, let's keep all dependencies listed. I will add them.

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

I forgot that enzyme is already included in @wordpress/scripts by design.

This comment has been minimized.

@youknowriad

youknowriad May 30, 2018

Contributor

I think we should be explicit :) If we use enzyme directly here in our tests, it should be a direct dependency

{
"name": "@wordpress/data",
"version": "0.0.1",
"description": "Data Redux module for WordPress",

This comment has been minimized.

@aduth

aduth May 29, 2018

Member

The documentation clearly states:

The data module is built upon and shares many of the same core principles of Redux, but shouldn't be mistaken as merely Redux for WordPress

Is this not in contradiction?

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

It is, I will update.

@@ -8,6 +8,18 @@ import deprecated from '@wordpress/deprecated';
*/
import { get } from 'lodash';
// Defaults to the local storage.

This comment has been minimized.

@aduth

aduth May 29, 2018

Member

So we default to a broken (error-throwing) state for Node environments where this package is used?

It's probably a larger dependency than we'd want, but something like Store.js is a good abstraction for this sort of thing.

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Not really, you can make it work by using setter method introduced in this PR:

import store from 'store';

setPersistenceStorage( store );

It isn't ideal, needs to be documented but should allow us to move forward. I will add the following example in the README file.

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Let's tackle it seperately as we don't have any docs for loadAndPersist anyways.

This comment has been minimized.

@gziolo

gziolo May 30, 2018

Member

Opened #7015 to solve it.

@gziolo

This comment has been minimized.

Member

gziolo commented May 30, 2018

Let's get it in and see how it goes 🎉

@gziolo gziolo merged commit c1fffb1 into master May 30, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +28.51% compared to bfafd1b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the update/data-package branch May 30, 2018

@gziolo gziolo modified the milestones: 3.1, 3.0 May 30, 2018

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