Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Issue 235 - Fix regressions introduced to PyPi and NPM packaging #237

Merged
merged 5 commits into from
Nov 9, 2018

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Nov 9, 2018

Fixes #235.

Reverting the /lib changes to go back to /dash_table.

In the end, changing dash_table/package.json to dash_table/package-info.json to allow npm to do its job correctly and to minimize impact.

This is the same solution as suggested here
plotly/dash-component-boilerplate#38

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-237 November 9, 2018 15:02 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-237 November 9, 2018 15:05 Inactive
package.json Outdated
"private::build:js-dev": "run-s \"private::build -- --mode development\"",
"private::build:js-test": "run-s \"private::build -- --config webpack.test.config.js\"",
"private::build:js-test-watch": "run-s \"private::build -- --config webpack.test.config.js --watch\"",
"private::build:py": "./extract-meta src/dash-table/DataTable.js > dash_table/metadata.json && cp package.json dash_table/bundle.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apart from going back to building in /dash_table, this is the major change, when copying package.json, rename it to something else

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-237 November 9, 2018 15:23 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-237 November 9, 2018 15:30 Inactive
@chriddyp
Copy link
Member

chriddyp commented Nov 9, 2018

FWIW, if you wanted to keep the previous lib/ structure, I believe that you could do so by just changing

'external_url': (
'https://unpkg.com/dash-table@{}/dash_table/bundle.js'
).format(__version__),

to

'https://unpkg.com/dash-table@{}/lib/bundle.js'.format(version)

@@ -13,7 +13,7 @@
_sys.exit(1)

_basepath = _os.path.dirname(__file__)
_filepath = _os.path.abspath(_os.path.join(_basepath, 'package.json'))
_filepath = _os.path.abspath(_os.path.join(_basepath, 'package-info.json'))
Copy link
Member

Choose a reason for hiding this comment

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

Whenever we want to include new non-Python files in the distribution, we need to add them to MANIFEST.in:

include dash_table/bundle.js
include dash_table/metadata.json
include dash_table/package.json
include README.md
include package.json

Python packaging is a white-list rather than a blacklist like .npmignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok cool, missed this comment when I asked at top level.

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

Just need the manifest.in change

@Marc-Andre-Rivet
Copy link
Contributor Author

@chriddyp I know. I'm ok with this with the other changes I suggested in the boilerplate.
MANIFEST.in, that's the list of files for the python package?

@chriddyp
Copy link
Member

chriddyp commented Nov 9, 2018

MANIFEST.in, that's the list of files for the python package?

Yes. It's the list of non-python files in the python package.

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit fbdf4cb into master Nov 9, 2018
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the 3.1-issue235-package branch July 18, 2019 12:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants