Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve JS facing packaging #38

Open
Marc-Andre-Rivet opened this issue Nov 9, 2018 · 9 comments
Open

Improve JS facing packaging #38

Marc-Andre-Rivet opened this issue Nov 9, 2018 · 9 comments

Comments

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Nov 9, 2018

Ran into some issues packaging the dash-table for JS consumption and would like to suggest making these modifications to the boilerplate in order to make the npm packages cleaner.

  1. Generate the package.json "main" property to be
  "main": {{cookiecutter.project_shortname}}/bundle.js

This makes import MyComponent from 'my-component' return the expected value (with libraryTarget='umd', most probably the component itself)

  1. Do away with blacklisting through .npmignore, use whitelisting instead through package.json "files" property

Default value could be

  "files": [
    "/{{cookiecutter.project_shortname}}/bundle*.js"
  ],

This would cover future bundle.dev.js, bundle.min.js, etc.
Package.json and *.md files are picked up automatically by npm.

  1. Modify the build:py script command to copy package.json into another file name (e.g. bundle.json) + update impacted setup.py and init.py files

This allows npm publish to pick up the file in {{cookiecutter.project_shortname}} correctly (otherwise since a package.json is present, the folder's content will be ignored -- configuring .npmignore to ignore that package.json will not modify this behavior)

  1. Do not promote the use of shrinkwrap for fully self-contained builds -- the shrinkwrap will override the peer & dev dependencies behavior and will install on wrapped dependencies on npm install

  2. Make it optional to set libraryTarget to 'umd' (maybe this is too specific to dash-table)

output: {
    libraryTarget: 'umd'
}

And modify react and react-dom external dependency resolution to cover the various use cases

        externals: {
            react: {
                commonjs: "react",
                commonjs2: "react",
                amd: "React",
                root: "React"
            },
            "react-dom": {
                commonjs: "react-dom",
                commonjs2: "react-dom",
                amd: "ReactDOM",
                root: "ReactDOM"
            }
        }
@nicolaskruchten
Copy link
Contributor

Re point 3, this would impact only __init__.py not setup.py, and it makes a lot of sense to me, although I'd call it package-info.json or something other than bundle.json. We could implement this change quite quickly by tweaking #33 which is still open atm.

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 12, 2018

Do away with blacklisting through .npmignore, use whitelisting instead through package.json "files" property

I think a blacklist is more appropriate for a boilerplate, if someone wants to adds more js/css files, they already have to modify the appropriate _dist in __init__ which is validated by the _validate_init.py. It would just make another place that risks failing and need more documentations/validations.

@nicolaskruchten
Copy link
Contributor

One thing to note is that there's not really a strong reason to publish Dash components on the NPM registry as far as I know, other than just making them available to a CDN like unpkg. What's the use-case for a non-Dash JS project to depend on a dash-component NPM module?

@nicolaskruchten
Copy link
Contributor

(and by the by, I think that serving the components locally by default makes more sense than relying on a CDN by default, thereby even further removing the need for NPM-publishing Dash components ;)

@nicolaskruchten
Copy link
Contributor

plotly/dash#284

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 12, 2018

@nicolaskruchten So set publish on npm to false by default ?

@nicolaskruchten
Copy link
Contributor

hmmm good question. I would tend to vote yes personally, as it's not really required if serving locally is the default.

@Marc-Andre-Rivet
Copy link
Contributor Author

@nicolaskruchten I agree with you that for most components npm publish seems mostly to be for the CDN. The dash-table is meant to be useable in a non-Dash environment but that's the exception more than the norm at the moment -- and if that's the case it makes some of the points above moot.

@nicolaskruchten
Copy link
Contributor

Yes it does seem that dash-table is the project with the unique requirements in this context.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants