Packages: Move element to packages maintained by Lerna #6756

gziolo · 2018-05-15T08:37:56Z

Description

Related issues: #3955, #6594.
Follow up for: #6658.

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 in #6658, this PR moves element to packages folder maintained by Lerna`.

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.

ntwb · 2018-05-15T09:00:59Z

packages/element/package.json

+{
+	"name": "@wordpress/element",
+	"version": "0.0.1",
+	"description": "Element module for WordPress",


Wondering if React should be included in the title for < cough >~~SEO~~</ cough > marketing/discoverability purposes?

Element React module for WordPress

ntwb

LGTM 👍

p.s. Not a blocker, but see my comment on maybe changing the package title

ntwb · 2018-05-15T11:35:33Z

Possible merge conflict with #6758 when it comes time to merge

youknowriad · 2018-05-15T12:52:53Z

package.json

@@ -20,7 +20,7 @@
 		"@wordpress/dom-ready": "1.0.4",
 		"@wordpress/hooks": "1.1.6",
 		"@wordpress/i18n": "1.1.0",
-		"@wordpress/is-shallow-equal": "1.0.1",


Any particular reason for this upgrade?

Yes, there were some issues with the package outside of Gutenberg and it wasn't bumped in Gutenberg. See: WordPress/packages#124.
We can do it separately.

No, I'm fine keeping it, just curious

youknowriad · 2018-05-15T12:54:12Z

packages/element/package.json

+		"@wordpress/is-shallow-equal": "1.0.2",
+		"lodash": "4.17.5",
+		"react": "16.3.2",
+		"react-dom": "16.3.2"


Does this mean, we can remove react and react-dom from the root level package.json?

Technically we can, but they won't be tracked in package-json.lock. In the previous PR we decided to keep them in sync with the root level package.json. We can revisit later. Personally, I would prefer to have it working with lock files and have only one place to include them.

I think it's not really important since it's just for test. But I agree it's a concern for other dependencies that are bundled and I think we should have a separate lock per module.

You are right, I forgot about it. As far as I remember, those local lock files, don't really work when using lerna bootstrap --hoist. @ntwb - or do they and we disabled to follow semantic versioning?

Hmmm, I've not got an accurate recollection, there was some trouble I think.

That said, https://github.com/WordPress/packages do not have lock files, why would the https://github.com/WordPress/gutenberg require lock files.

tl;dr: Lock files are for apps, not packages or modules

I understand your point but I think I disagree. The difference between "app" and "packages" is superficial. What's the app when your whole app is a collection of packages? How do you guarantee an npm install won't inadvertently break your application?

You can't which is one of my points in that if you look down an "app" or "package" with specific package versions using a lock file then what is the point of SemVer, you may as well release everything as a major release and never worry about patch or minor releases for an "app" or "package" that is depended upon

So you're ok with merging a PR and than your automatic CI job releasing your packages/app break because it goes with the wrong package you never tested?

I'm not. I see where you're coming from but that's true when dependencies get bundled together to build a single bundle which is not the case in our setup, each package is an app that generates its own script. We need predictability.

On a "package" side of things I am happy to let SemVer do what it was designed to do.

On an "app" I'm with you 100% and everything should be locked down.

Anything in between is tricky, which is where we are at here...

If the Gutenberg packages are released using SemVer then using an explicit locked down version e.g. 1.0.0 and not ~1.0.0 or ^1.0.0 of that package for Gutenberg with a lock file will work perfectly for Gutenberg as the only time a dependancy is bumped is when it is manually bumped which would include testing that the version bump works as expected.

Individual packages are then free to be used in the wider community and will continue to adhere to SemVer allowing other packages to depend upon them and update as they see fit per their own SemVer policy.

Anyways, that's my take, should probably save this discussion for the follow up PR @gziolo has planned 😄

But I'm still not sure I follow you. There's no difference between an "app" and a "package". All Gutenberg packages are "apps" because they're all released as such as WordPress scripts.

Gutenberg will never "require" or depend on Gutenberg packages because they're built in the same repository. That may change (not certain) when the merge happens but until then, all the packages are apps, there's no higher-level package-lock.json where they can be locked.

gziolo · 2018-05-15T13:38:10Z

Let's merge this one. I will open a follow-up to play with the lock files.

aduth · 2018-05-23T18:19:27Z

test/unit/setup-wp-aliases.js

-		get: () => require( entryPointName ),
-	} );
+Object.defineProperty( global.wp, 'element', {
+	get: () => require( 'packages/element' ),


How is it that packages/element refers to the packages/element folder, when it's not being defined as a path (and even if it were, required from test/unit)?

Jest resolves those paths starting from the root dir, that's why it works.

Our goal is to get rid of this workaround completely when we have all modules moved to packages and we enforce using createElement in files using JSX - related commit 69af0a4 from #6828.

The same applies to wb.blocks which is here temporarily until we remove deprecations which use this global explicitly.

Packages: Move element to packages maintained by Lerna

f1a9e7b

gziolo requested review from youknowriad and ntwb May 15, 2018 08:38

gziolo self-assigned this May 15, 2018

gziolo added the npm Packages Related to npm packages label May 15, 2018

gziolo added this to the 2.9 milestone May 15, 2018

gziolo added [Package] Element /packages/element [Type] Refactoring labels May 15, 2018

ntwb reviewed May 15, 2018

View reviewed changes

ntwb approved these changes May 15, 2018

View reviewed changes

Packages: Add React keyword to the description

b333b22

ntwb mentioned this pull request May 15, 2018

Packages: Extract dom package and make it maintained with Lerna #6758

Merged

3 tasks

youknowriad reviewed May 15, 2018

View reviewed changes

gziolo merged commit cd848e9 into master May 15, 2018

gziolo deleted the update/element-package-extract branch May 15, 2018 13:38

This was referenced May 18, 2018

Packages: Move data module to the package maintained by Lerna #6828

Merged

Deprecated: Introduce new module with depreaction util #6914

Merged

aduth reviewed May 23, 2018

View reviewed changes

gziolo mentioned this pull request May 28, 2018

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

Merged

3 tasks

Shelob9 mentioned this pull request May 29, 2018

Packages: Publish all modules as independent npm packages #3955

Closed

19 tasks

ellatrix added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Code Quality Issues or PRs that relate to code quality labels Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packages: Move element to packages maintained by Lerna #6756

Packages: Move element to packages maintained by Lerna #6756

gziolo commented May 15, 2018

ntwb May 15, 2018

ntwb left a comment

ntwb commented May 15, 2018

youknowriad May 15, 2018

gziolo May 15, 2018

youknowriad May 15, 2018

youknowriad May 15, 2018

gziolo May 15, 2018

youknowriad May 15, 2018

gziolo May 15, 2018 •

edited

Loading

ntwb May 15, 2018

youknowriad May 15, 2018

ntwb May 15, 2018

youknowriad May 15, 2018 •

edited

Loading

ntwb May 15, 2018

youknowriad May 15, 2018

gziolo commented May 15, 2018

aduth May 23, 2018

gziolo May 24, 2018

Packages: Move element to packages maintained by Lerna #6756

Packages: Move element to packages maintained by Lerna #6756

Conversation

gziolo commented May 15, 2018

Description

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

ntwb left a comment

Choose a reason for hiding this comment

ntwb commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo May 15, 2018 •

edited

Loading

youknowriad May 15, 2018 •

edited

Loading