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

Projects
None yet
3 participants
@gziolo
Member

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 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 WordPress/gutenberg-core May 31, 2018

@gziolo gziolo referenced this pull request May 31, 2018

Closed

Packages: Publish all modules as independent npm packages #3955

19 of 19 tasks complete
@ntwb

ntwb approved these changes May 31, 2018

@ntwb

This comment has been minimized.

Member

ntwb commented May 31, 2018

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

@aduth

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

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.

@gziolo gziolo merged commit 26ebf42 into master May 31, 2018

2 checks passed

codecov/project 46.31% (-0.16%) compared to 6728906
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the update/packages-main-module branch May 31, 2018

@gziolo gziolo modified the milestones: 3.1, 3.0 May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment