chore(build): add distribution files #186

MediumRareByte · 2017-11-20T12:04:20Z

Added distribution bundle.
#142

alexreardon

This is a simple way of building a dist - i like it! However, can we please not add the generated file to the repo itself? Ideally it would just be generated for npm (just as lib is)

Based on this understanding here are the outstanding tasks:

add dist to package.json#files
add dist to .gitignore
remove the dist directory from this commit

alexreardon · 2017-11-20T21:02:29Z

Does this naming convention satisfy the Clojurescript requirements? @Frozenlock?

Frozenlock · 2017-11-20T21:23:00Z

Thank you for doing this.

IIRC as long as one can easily generate a minified file for distribution we can package it with a minimum of manual operations.

I'll try to look at this in more details this evening and come back with an answer.

alexreardon · 2017-11-20T22:54:15Z

I think we also need to ensure that we are bundling react

alexreardon · 2017-11-20T23:02:25Z

Also, there is a lot of webpack stuff in there that we could avoid if we used rollup. It seems fairly easy to setup also - and we could use /lib/index.js as the entry point. It also seems to have simple options for peer dependencies

Frozenlock · 2017-11-21T04:31:12Z

After reviewing the process, the thing we need is the browserified files (like those).

We provide our own packaged dependencies for the most popular libraries (React, react-motion, etc.).
This means that if they are in the externals section of the webpack.config.js (or equivalent), we can avoid duplication.

However, I don't know enough about the vanilla JS ecosystem nor do I want to burden this project.
I would simply ask that you do distribution files as you normally would; we'll handle it from there.

MediumRareByte · 2017-11-21T12:00:07Z

Swap out Webpack for Rollout
add dist to package.json#files
add dist to .gitignore
remove the dist directory from this commit

This is my first time using Rollup, I followed the implementation mentioned in #81

alexreardon · 2017-11-21T21:16:35Z

This is looking good. @treshugart does it look good to you?

@Frozenlock would this satisfy your requirements?

MediumRareByte · 2017-11-21T21:26:17Z

Thank you @alexreardon. Please let me know if that suits your bill, @treshugart @Frozenlock :)

treshugart · 2017-11-21T22:33:08Z

package.json

-  "files": [
-    "/lib"
-  ],
+  "files": ["/lib", "/dist"],


Would be good to also add a browser field next to main that maps to dist/index.js (it will be minified by bundlers, however unpkg won't serve the min file).

treshugart · 2017-11-21T22:33:59Z

LGTM but left a comment that we might think about adding now. Some things I'd suggest thinking about for the future:

dist/ is really umd/ and can map to browser (I've commented about mapping the current dist/ to that)
lib/ is really node/ and can map to main
es/ (or even esm/ now that I think about it) that exports es5 (or lowest common denominator) + esm and can map to module
Having an esnext/ build for consuming the latest spec is something people are doing more and more. I've done this in Skate and map it to the esnext field.

alexreardon · 2017-11-21T23:04:06Z

@treshugart can we implement your suggestions in a follow up issue? It seems like the currently PR solves the immediate need

treshugart · 2017-11-22T01:12:26Z

Some things I'd suggest thinking about for the future:

treshugart · 2017-11-22T01:13:27Z

Or if you mean the browser field, that's up to if you think people might be reaching into the dist manually and if you want to consider a folder rename a breaking change. I don't care what you do here.

alexreardon

I had a look and it looks as though react is still being included in the generated files.

A few things I think should still be done:

Add React as an external: https://rollupjs.org/#peer-dependencies
Add the babel plugin and exclude node modules from needing to be transpiled
Add build:dist to build command

alexreardon · 2017-11-24T04:08:42Z

rollup.config.js

+  name: 'ReactBeautifulDnd',
+  plugins: [
+    resolve({
+      browser: true,


why is this needed?

// some package.json files have a `browser` field which // specifies alternative files to load for people bundling // for the browser. If that's you, use this option, otherwise // pkg.browser will be ignored browser: true, // Default: false

alexreardon · 2017-11-24T04:09:14Z

rollup.config.js

+  name: 'ReactBeautifulDnd',
+  plugins: [
+    resolve({
+      browser: true,


it looks like this already defaults to true

// use "main" field or index.js, even if it's not an ES6 module // (needs to be converted from CommonJS to ES6 // – see https://github.com/rollup/rollup-plugin-commonjs main: true, // Default: true

alexreardon · 2017-11-24T04:09:52Z

rollup.config.js

+      browser: true,
+      main: true,
+    }),
+    commonjs({


not sure what this one is for. I think just listing react as an external is enough

rollup-plugin-commonjs complains that React does not export 'Component', 'Children', 'createElement' as named exports. This was the solution I've found.

Listing React as an external dependency leads to
(!) Missing global variable name Use options.globals to specify browser global variable names corresponding to external modules react (guessing '_react')
I'm not sure if that's fine, or should I include globals: { react: 'React' } to rollup.config.js?

Yeah, I think you're going to have to do that. I've found you can automate the dependencies part by doing something like:

const pkg = require('./package.json'); const skip = { ...pkg.dependencies, ...pkg.devDependencies, ...pkg.optionalDependencies, ...pkg.peerDependencies };

Jason Miller (of Preact fame) also shows this here.

alexreardon · 2017-11-26T02:56:20Z

This is looking good! I will give it a look over next week.

MediumRareByte · 2017-11-26T04:17:27Z

Thanks @alexreardon

alexreardon · 2017-11-26T20:56:01Z

rollup.config.js

+import pkg from './package.json';
+
+const args = yargs.argv;
+const external = Object.keys(pkg.dependencies || {}).concat(


Getting close! I think this should only include react. The other dependencies are internal and should not need to be provided by consumers.

Also, I am getting this warning:

(!) Missing global variable names Use options.globals to specify browser global variable names corresponding to external modules react (guessing '_react')

I would expect that this should be listed as 'React' in the options.globals

alexreardon · 2017-11-26T20:56:33Z

Thanks for preserving with this @vinifala

MediumRareByte · 2017-11-26T21:46:20Z

Thanks @alexreardon, React added as only external dependency with proper global mapping :)

MediumRareByte · 2017-11-26T21:47:46Z

From Tavis CI: oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded"
https://travis-ci.org/atlassian/react-beautiful-dnd/builds/307622143?utm_source=github_status&utm_medium=notification
Looks like we ran out of space.

alexreardon · 2017-11-26T21:55:25Z

Oh wow! I am not too worried about that at this stage

alexreardon · 2017-11-27T01:53:27Z

Well done @vinifala !! I had a look locally and it is looking good. I'll merge this pr and it will go out in the next release. Thanks for your ongoing efforts to get this across the line 🎉

MediumRareByte · 2017-11-27T02:16:26Z

Thanks for your patience, @alexreardon. I'm looking forward to contribute further.

alexreardon requested changes Nov 20, 2017

View reviewed changes

MediumRareByte force-pushed the add-dist-files branch from 46ebc7b to eff5f8a Compare November 21, 2017 11:56

treshugart reviewed Nov 21, 2017

View reviewed changes

alexreardon requested changes Nov 24, 2017

View reviewed changes

alexreardon reviewed Nov 24, 2017

View reviewed changes

MediumRareByte force-pushed the add-dist-files branch from 86338c6 to f7f70d3 Compare November 25, 2017 23:32

alexreardon requested changes Nov 26, 2017

View reviewed changes

chore(build): add react as only external dependency

f567a85

MediumRareByte force-pushed the add-dist-files branch from f7f70d3 to f567a85 Compare November 26, 2017 21:45

Merge branch 'master' into add-dist-files

a4c249e

alexreardon approved these changes Nov 27, 2017

View reviewed changes

alexreardon merged commit b61813d into atlassian:master Nov 27, 2017

MediumRareByte deleted the add-dist-files branch November 27, 2017 02:16

alexreardon mentioned this pull request Nov 27, 2017

Distribution files? #142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(build): add distribution files #186

chore(build): add distribution files #186

MediumRareByte commented Nov 20, 2017

alexreardon left a comment

alexreardon commented Nov 20, 2017

Frozenlock commented Nov 20, 2017

alexreardon commented Nov 20, 2017

alexreardon commented Nov 20, 2017 •

edited

Loading

Frozenlock commented Nov 21, 2017 •

edited

Loading

MediumRareByte commented Nov 21, 2017 •

edited

Loading

alexreardon commented Nov 21, 2017

MediumRareByte commented Nov 21, 2017

treshugart Nov 21, 2017 •

edited

Loading

treshugart commented Nov 21, 2017 •

edited

Loading

alexreardon commented Nov 21, 2017

treshugart commented Nov 22, 2017

treshugart commented Nov 22, 2017 •

edited

Loading

alexreardon left a comment

alexreardon Nov 24, 2017

alexreardon Nov 24, 2017

alexreardon Nov 24, 2017

MediumRareByte Nov 24, 2017

MediumRareByte Nov 24, 2017

treshugart Nov 25, 2017

alexreardon commented Nov 26, 2017

MediumRareByte commented Nov 26, 2017

alexreardon Nov 26, 2017

alexreardon commented Nov 26, 2017

MediumRareByte commented Nov 26, 2017

MediumRareByte commented Nov 26, 2017 •

edited

Loading

alexreardon commented Nov 26, 2017

alexreardon commented Nov 27, 2017

MediumRareByte commented Nov 27, 2017

chore(build): add distribution files #186

chore(build): add distribution files #186

Conversation

MediumRareByte commented Nov 20, 2017

alexreardon left a comment

Choose a reason for hiding this comment

alexreardon commented Nov 20, 2017

Frozenlock commented Nov 20, 2017

alexreardon commented Nov 20, 2017

alexreardon commented Nov 20, 2017 • edited Loading

Frozenlock commented Nov 21, 2017 • edited Loading

MediumRareByte commented Nov 21, 2017 • edited Loading

alexreardon commented Nov 21, 2017

MediumRareByte commented Nov 21, 2017

treshugart Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

treshugart commented Nov 21, 2017 • edited Loading

alexreardon commented Nov 21, 2017

treshugart commented Nov 22, 2017

treshugart commented Nov 22, 2017 • edited Loading

alexreardon left a comment

Choose a reason for hiding this comment

alexreardon Nov 24, 2017

Choose a reason for hiding this comment

alexreardon Nov 24, 2017

Choose a reason for hiding this comment

alexreardon Nov 24, 2017

Choose a reason for hiding this comment

MediumRareByte Nov 24, 2017

Choose a reason for hiding this comment

MediumRareByte Nov 24, 2017

Choose a reason for hiding this comment

treshugart Nov 25, 2017

Choose a reason for hiding this comment

alexreardon commented Nov 26, 2017

MediumRareByte commented Nov 26, 2017

alexreardon Nov 26, 2017

Choose a reason for hiding this comment

alexreardon commented Nov 26, 2017

MediumRareByte commented Nov 26, 2017

MediumRareByte commented Nov 26, 2017 • edited Loading

alexreardon commented Nov 26, 2017

alexreardon commented Nov 27, 2017

MediumRareByte commented Nov 27, 2017

alexreardon commented Nov 20, 2017 •

edited

Loading

Frozenlock commented Nov 21, 2017 •

edited

Loading

MediumRareByte commented Nov 21, 2017 •

edited

Loading

treshugart Nov 21, 2017 •

edited

Loading

treshugart commented Nov 21, 2017 •

edited

Loading

treshugart commented Nov 22, 2017 •

edited

Loading

MediumRareByte commented Nov 26, 2017 •

edited

Loading