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

Fix files configuration entry in package.json for @wordpress/babel-preset-default #7933

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Jul 12, 2018

Description

The package doesn’t have build and build-module directories so the published entrypoint should be index.js.

When I went to install the package in a plugin it was installed with no index.js entrypoint in the node_modules/wordpress/babel-preset-default folder. Thus all builds of my plugin failed with:

Module build failed (from ./node_modules/babel-loader/lib/index.js):
    Error: Cannot find module '@wordpress/babel-preset-default' from '/Users/dethier/webprojects/vagrant/www/ee-alpha-testing/wp-content/plugins/event-espresso-core'

For testing this will likely require just doing the various npm script runs.

The published package is currently broken because of this so another release will need done.

The package doesn’t have `build` and `build-module` directories so the published entrypoint should be `index.js`.
@nerrad nerrad requested a review from gziolo July 12, 2018 14:59
@nerrad nerrad self-assigned this Jul 12, 2018
@nerrad nerrad added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling [Package] Babel preset /packages/babel-preset-default labels Jul 12, 2018
@nerrad nerrad requested review from ntwb and aduth July 12, 2018 15:00
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@aduth
Copy link
Member

aduth commented Jul 12, 2018

Looking through other packages for potential issues...

  • browserslist-config offers an index.js
    • Has no files and points main to index.js (would be inferred anyways), so should be fine
  • jest-preset-default offers an index.js
    • files does not include index.js
    • But: It is in the published package. I'm guessing npm includes main file as well automatically?
  • npm-package-json-lint-config offers an index.js
    • Has no files and points main to index.js (would be inferred anyways), so should be fine
  • scripts has no build, offers scripts and utils folders
    • Included in its files, so should be fine

@gziolo
Copy link
Member

gziolo commented Jul 13, 2018

The package doesn’t have build and build-module directories so the published entrypoint should be index.js.

My bad, sorry about that. I'm merging and will publish another version soon 👍

@gziolo
Copy link
Member

gziolo commented Jul 13, 2018

@aduth according to https://docs.npmjs.com/files/package.json#files:

Certain files are always included, regardless of settings:

package.json
README
CHANGES / CHANGELOG / HISTORY
LICENSE / LICENCE
NOTICE
The file in the "main" field

@gziolo gziolo merged commit e4ac2f4 into master Jul 13, 2018
@gziolo gziolo deleted the bug/fix-package.json-for-babel-preset-default-package branch July 13, 2018 06:29
@gziolo gziolo added this to the 3.3 milestone Jul 13, 2018
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 [Package] Babel preset /packages/babel-preset-default [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants