Repository navigation
theme: Validate npm publish surface #79552
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
Merged
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
e325c79
theme: Clean npm publish surface
ciampo 4f7b989
theme: Add publish surface changelog entry
ciampo cc3f1d9
theme: Use patterns for package files
ciampo 6307dd7
theme: Narrow published declarations
ciampo 7ac543e
theme: Add package contents validation
ciampo c47bcfe
theme: Simplify package contents validation
ciampo a6a9ae9
theme: Strengthen package contents validation
ciampo 8097604
theme: Fix package validation on Windows
ciampo 4ac9744
theme: Move package contents validation to linting
ciampo 8d16432
theme: Validate packed package types with attw
ciampo 6ecec6b
theme: Address package validation feedback
ciampo 4ee380f
theme: Build package-only declarations
ciampo 841ed52
tools: Run package contents tests with Jest
ciampo c99b5c1
theme: Remove stale Sass export
ciampo 605b7c6
tools: Address package validation feedback
ciampo 3330935
theme: Adapt package validation to ESM
ciampo dd7df8c
theme: Bundle public package types
ciampo 52a93da
validation: Harden package contents cleanup
ciampo 3aba7f6
theme: Resolve package types during clean builds
ciampo fd3d0cd
theme: Bundle package types before packing
ciampo a061e7c
theme: Simplify package publish validation
ciampo d966645
validation: Exclude singular story files
ciampo 59173df
validation: Let packages opt into publish checks
ciampo 8850a7b
theme: Streamline package publish checks
ciampo 7125481
theme: Remove attw package validation
ciampo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import { spawnSync } from 'node:child_process'; | ||
| import { readFileSync } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join, resolve } from 'node:path'; | ||
|
|
||
| const [ packagePath, ...extraArgs ] = process.argv.slice( 2 ); | ||
|
|
||
| if ( ! packagePath || extraArgs.length ) { | ||
| console.error( | ||
| 'Usage: node tools/validation/validate-package-contents.mjs <package-directory>' | ||
| ); | ||
| process.exit( 1 ); | ||
| } | ||
|
|
||
| const packageRoot = resolve( process.cwd(), packagePath ); | ||
| const packageJson = JSON.parse( | ||
| readFileSync( join( packageRoot, 'package.json' ), 'utf8' ) | ||
| ); | ||
| const npmExecPath = process.env.npm_execpath; | ||
| const packResult = spawnSync( | ||
| npmExecPath ? process.execPath : 'npm', | ||
| [ | ||
| ...( npmExecPath ? [ npmExecPath ] : [] ), | ||
| 'pack', | ||
| '--dry-run', | ||
| '--json', | ||
| ], | ||
| { | ||
| cwd: packageRoot, | ||
| encoding: 'utf8', | ||
| env: { | ||
| ...process.env, | ||
| npm_config_cache: | ||
| process.env.WORDPRESS_PACKAGE_NPM_CACHE ?? | ||
| join( tmpdir(), 'wordpress-package-npm-cache' ), | ||
| }, | ||
| shell: ! npmExecPath && process.platform === 'win32', | ||
| } | ||
| ); | ||
|
|
||
| if ( packResult.error ) { | ||
| throw packResult.error; | ||
| } | ||
|
|
||
| if ( packResult.status !== 0 ) { | ||
| process.stderr.write( packResult.stdout ?? '' ); | ||
| process.stderr.write( packResult.stderr ?? '' ); | ||
| process.exit( packResult.status ?? 1 ); | ||
| } | ||
|
|
||
| const [ pack ] = JSON.parse( packResult.stdout ); | ||
| const packedPaths = pack.files.map( ( { path } ) => path ); | ||
| const packedPathSet = new Set( packedPaths ); | ||
| const disallowedPathPatterns = [ | ||
| /(^|\/)(__fixtures__|__snapshots__|__tests__|fixtures|stories|test|tests)(\/|$)/, | ||
| /(^|\/)[^/]+\.(spec|test)\.[^/]+$/, | ||
| /(^|\/)[^/]+\.stor(?:y|ies)\.[^/]+$/, | ||
| ]; | ||
| const disallowedPaths = packedPaths.filter( ( path ) => | ||
| disallowedPathPatterns.some( ( pattern ) => pattern.test( path ) ) | ||
| ); | ||
|
|
||
| /** | ||
| * @param {unknown} value Export map value. | ||
| * @return {string[]} Concrete local export target paths. | ||
| */ | ||
| function getLocalExportTargets( value ) { | ||
| if ( typeof value === 'string' ) { | ||
| return value.startsWith( './' ) && ! value.includes( '*' ) | ||
| ? [ value.slice( 2 ) ] | ||
| : []; | ||
| } | ||
|
|
||
| if ( Array.isArray( value ) ) { | ||
| return value.flatMap( getLocalExportTargets ); | ||
| } | ||
|
|
||
| if ( ! value || typeof value !== 'object' ) { | ||
| return []; | ||
| } | ||
|
|
||
| return Object.values( value ).flatMap( getLocalExportTargets ); | ||
| } | ||
|
|
||
| const missingExportTargets = getLocalExportTargets( | ||
| packageJson.exports | ||
| ).filter( ( path ) => ! packedPathSet.has( path ) ); | ||
|
|
||
| if ( packedPaths.length === 0 ) { | ||
| console.error( 'The package tarball does not include any files.' ); | ||
| } | ||
|
|
||
| if ( disallowedPaths.length ) { | ||
| console.error( | ||
| [ | ||
| 'The package tarball includes disallowed files:', | ||
| ...disallowedPaths.map( ( path ) => `- ${ path }` ), | ||
| ].join( '\n' ) | ||
| ); | ||
| } | ||
|
|
||
| if ( missingExportTargets.length ) { | ||
| console.error( | ||
| [ | ||
| 'The package tarball is missing exported targets:', | ||
| ...missingExportTargets.map( ( path ) => `- ${ path }` ), | ||
| ].join( '\n' ) | ||
| ); | ||
| } | ||
|
|
||
| if ( | ||
| packedPaths.length === 0 || | ||
| disallowedPaths.length || | ||
| missingExportTargets.length | ||
| ) { | ||
| process.exit( 1 ); | ||
| } | ||
|
|
||
| console.log( | ||
| `Validated ${ packedPaths.length } packed files for ${ | ||
| packageJson.name ?? packageRoot | ||
| }.` | ||
| ); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| /* global afterEach, expect, test */ | ||
|
|
||
| /** | ||
| * External dependencies | ||
| */ | ||
| const { spawnSync } = require( 'node:child_process' ); | ||
| const { mkdtempSync, mkdirSync, rmSync, writeFileSync } = require( 'node:fs' ); | ||
| const { tmpdir } = require( 'node:os' ); | ||
| const { dirname, join } = require( 'node:path' ); | ||
|
|
||
| const validatorPath = join( __dirname, 'validate-package-contents.mjs' ); | ||
| const temporaryRoots = []; | ||
|
|
||
| afterEach( () => { | ||
| for ( const root of temporaryRoots.splice( 0 ) ) { | ||
| rmSync( root, { force: true, recursive: true } ); | ||
| } | ||
| } ); | ||
|
|
||
| function createPackage( { files, packageJson } ) { | ||
| const root = mkdtempSync( join( tmpdir(), 'validate-package-contents-' ) ); | ||
| temporaryRoots.push( root ); | ||
|
|
||
| for ( const [ path, contents ] of Object.entries( files ) ) { | ||
| const filePath = join( root, path ); | ||
| mkdirSync( dirname( filePath ), { recursive: true } ); | ||
| writeFileSync( filePath, contents ); | ||
| } | ||
|
|
||
| writeFileSync( | ||
| join( root, 'package.json' ), | ||
| JSON.stringify( | ||
| { | ||
| name: 'test-package', | ||
| version: '1.0.0', | ||
| ...packageJson, | ||
| }, | ||
| null, | ||
| '\t' | ||
| ) + '\n' | ||
| ); | ||
|
|
||
| return root; | ||
| } | ||
|
|
||
| function runValidator( packageRoot ) { | ||
| return spawnSync( process.execPath, [ validatorPath, packageRoot ], { | ||
| encoding: 'utf8', | ||
| env: { | ||
| ...process.env, | ||
| WORDPRESS_PACKAGE_NPM_CACHE: join( | ||
| tmpdir(), | ||
| 'wordpress-package-npm-cache' | ||
| ), | ||
| }, | ||
| } ); | ||
| } | ||
|
|
||
| test( 'passes for a package with clean packed contents', () => { | ||
| const packageRoot = createPackage( { | ||
| files: { | ||
| 'index.js': "export const value = 'ok';\n", | ||
| 'index.d.ts': 'export declare const value: string;\n', | ||
| 'styles.css': '.test {}\n', | ||
| }, | ||
| packageJson: { | ||
| files: [ 'index.js', 'index.d.ts', 'styles.css' ], | ||
| exports: { | ||
| '.': { | ||
| types: './index.d.ts', | ||
| default: './index.js', | ||
| }, | ||
| './styles.css': './styles.css', | ||
| }, | ||
| }, | ||
| } ); | ||
|
|
||
| const result = runValidator( packageRoot ); | ||
|
|
||
| expect( result.status ).toBe( 0 ); | ||
| expect( result.stdout ).toMatch( | ||
| /Validated \d+ packed files for test-package\./ | ||
| ); | ||
| } ); | ||
|
|
||
| test.each( [ 'index.test.js', 'index.story.js' ] )( | ||
| 'fails when packed contents include %s', | ||
| ( disallowedPath ) => { | ||
| const packageRoot = createPackage( { | ||
| files: { | ||
| 'index.js': "export const value = 'ok';\n", | ||
| [ disallowedPath ]: "import './index.js';\n", | ||
| }, | ||
| packageJson: { | ||
| files: [ 'index.js', disallowedPath ], | ||
| exports: './index.js', | ||
| }, | ||
| } ); | ||
|
|
||
| const result = runValidator( packageRoot ); | ||
|
|
||
| expect( result.status ).not.toBe( 0 ); | ||
| expect( result.stderr ).toContain( | ||
| `The package tarball includes disallowed files:\n- ${ disallowedPath }` | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| test( 'fails when an exported target is missing from the package', () => { | ||
| const packageRoot = createPackage( { | ||
| files: { | ||
| 'index.js': "export const value = 'ok';\n", | ||
| }, | ||
| packageJson: { | ||
| files: [ 'index.js' ], | ||
| exports: { | ||
| '.': { | ||
| types: './index.d.ts', | ||
| default: './index.js', | ||
| }, | ||
| }, | ||
| }, | ||
| } ); | ||
|
|
||
| const result = runValidator( packageRoot ); | ||
|
|
||
| expect( result.status ).not.toBe( 0 ); | ||
| expect( result.stderr ).toMatch( | ||
| /The package tarball is missing exported targets:\n- index\.d\.ts/ | ||
| ); | ||
| } ); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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 wonder if we should work toward
srcbeing a disallowed path. I expect we have some references that we need it for now, but it feels like if we're building something fromsrc, we should always be referencing its built output in the runtime usage. And if something truly doesn't need to be transpiled at all (e.g. plugins) then we shouldn't go through the effort of building them and they could live outside the source folder.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 agree with this direction. For
@wordpress/theme, most of thesesrcpaths already have built equivalents: the plugin and design-token JavaScript underbuild-module, and the CSS underbuild-style. The remaining complication is the hand-authored plugin declarations currently shipped fromsrc.I think the clean follow-up is to generate those declarations into
build-types, point all runtime and style exports at the built directories, and then removesrcfrom the package entirely. I wouldn't makesrcglobally disallowed here because other Gutenberg packages intentionally publish source assets, but we can enforce it specifically for@wordpress/themeonce that cleanup lands?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.
Opened a draft #80213 , will iterate on it