-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 element to packages maintained by Lerna #6756
Conversation
packages/element/package.json
Outdated
{ | ||
"name": "@wordpress/element", | ||
"version": "0.0.1", | ||
"description": "Element module for WordPress", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if React
should be included in the title for < cough >SEO</ cough > marketing/discoverability purposes?
Element React module for WordPress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
p.s. Not a blocker, but see my comment on maybe changing the package title
Possible merge conflict with #6758 when it comes time to merge |
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for this upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm fine keeping it, just curious
"@wordpress/is-shallow-equal": "1.0.2", | ||
"lodash": "4.17.5", | ||
"react": "16.3.2", | ||
"react-dom": "16.3.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean, we can remove react
and react-dom
from the root level package.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Let's merge this one. I will open a follow-up to play with the lock files. |
get: () => require( entryPointName ), | ||
} ); | ||
Object.defineProperty( global.wp, 'element', { | ||
get: () => require( 'packages/element' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 moveselement
topackages folder maintained by
Lerna`.How has this been tested?
Manually:
node_modules
folder.npm install
npm test
npm run dev
npm run build
npm run package-plugin
Types of changes
Refactoring.
Checklist: