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

Packages: Always publish main and module distributions #7047

Merged
merged 1 commit into from
May 31, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 31, 2018

Description

Reported by @Shelob9:
I'm getting an error when importing the data package, or requiring it. I'm using create-react-app.

const data = require( '@wordpress/data' ); and import { data } from '@wordpress/data'; result in Jest throwing an error like this:

/caldera-grid/node_modules/@wordpress/data/src/index.js:4
    import { combineReducers, createStore } from 'redux';
           ^
    
    SyntaxError: Unexpected token {

The issue was on my side. I didn't set main and module inside package.json files properly. This PR fixes it.

How has this been tested?

npm run dev and npm run build still work properly.

@youknowriad, can you confirm that postcss-themes still works as expected?

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended npm Packages Related to npm packages labels May 31, 2018
@gziolo gziolo added this to the 3.1 milestone May 31, 2018
@gziolo gziolo self-assigned this May 31, 2018
@gziolo gziolo requested review from youknowriad, ntwb and a team May 31, 2018 10:08
@ntwb
Copy link
Member

ntwb commented May 31, 2018

As an aside, the npm-package-json-lint rule 'require-module': 'on', sould be added to #7001

@aduth
Copy link
Member

aduth commented May 31, 2018

As an aside, the npm-package-json-lint rule 'require-module': 'on', sould be added to #7001

What about where we don't want to use module ? Or a built main ? See also WordPress/packages#124 . Should we just not support this?

@ntwb
Copy link
Member

ntwb commented May 31, 2018

What about where we don't want to use module ? Or a built main ? See also WordPress/packages#124 . Should we just not support this?

Fair call, I based my comment on the issue title "Always publish main and module distributions"

I couldn't find the reference but there was a brief discussion someplace on re-introducing and standardizing all packages in a /src folder and building out to /build.

@gziolo gziolo force-pushed the update/packages-main-module branch from 7bc4330 to 1b42aa4 Compare May 31, 2018 18:00
@gziolo
Copy link
Member Author

gziolo commented May 31, 2018

I agree that module should be optional. In fact, I removed module entry from package.json files from both packages which are not meant to be executed in the browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants