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 npm org scope to WordPress dependencies #2172

Merged
merged 2 commits into from
Aug 3, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 3, 2017

Related: #941
Related: #1205

This pull request seeks to add the WordPress npm organization scope to all WordPress dependency imports. It is a further realization of the intent for these top-level folders to be distributed as independent packages.

Other side-benefits include:

  • Surfaces pathed imports on WordPress dependencies
  • Surfaces circular dependencies
  • Surfaces incorrectly specified wp_register_script script dependencies
  • Eliminates chance of naming conflicts between "WordPress dependencies" and other npm or built-in modules
  • Reduces overall minified bundle size by 8.7% (1285kb -> 1124kb, related to first point and Module and import strategy #941)

Implementation notes:

This will be hellish to rebase, so I'd prefer not to let this one linger.

Testing instructions:

This is difficult to test because it touches nearly every file in the application. Verify through npm run build, npm run dev, npm test that no errors occur, and that no regressions or console errors occur by thorough use of the application.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Aug 3, 2017
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Awesome. Ship it, it works great 👍

@aduth aduth force-pushed the update/wordpress-scoped-dependencies branch from 8314260 to cf5a450 Compare August 3, 2017 12:37
@codecov
Copy link

codecov bot commented Aug 3, 2017

Codecov Report

Merging #2172 into master will increase coverage by 0.14%.
The diff coverage is 7.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2172      +/-   ##
=========================================
+ Coverage   22.15%   22.3%   +0.14%     
=========================================
  Files         138     137       -1     
  Lines        4283    4291       +8     
  Branches      723     722       -1     
=========================================
+ Hits          949     957       +8     
- Misses       2814    2815       +1     
+ Partials      520     519       -1
Impacted Files Coverage Δ
components/button/index.js 90.9% <ø> (ø) ⬆️
blocks/library/embed/index.js 45.97% <ø> (ø) ⬆️
blocks/library/list/index.js 8.21% <ø> (ø) ⬆️
blocks/inspector-controls/toggle-control/index.js 0% <ø> (ø) ⬆️
blocks/library/more/index.js 30% <ø> (ø) ⬆️
blocks/library/cover-text/index.js 30% <ø> (ø) ⬆️
blocks/block-controls/index.js 0% <ø> (ø) ⬆️
components/higher-order/with-focus-return/index.js 100% <ø> (ø) ⬆️
blocks/library/html/index.js 23.07% <ø> (ø) ⬆️
blocks/block-alignment-toolbar/index.js 33.33% <ø> (ø) ⬆️
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2beeb4c...cf5a450. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants