chore(build): Add esm, cjs, umd & dist build targets #14

benmvp · 2018-03-28T22:00:33Z

Added Gulp in order to create build targets for ECMAScript modules (ESM), CommonJS (cjs), Universal Modules (UMD), and dist bundles. This should hopefully be the final step before actually doing an inaugural release.

ESM - used by modern dependency systems like Webpack or Rollup that can do tree-shaking optimizations
CJS - used primarily by Node for resolving dependencies
Dist bundle - used by legacy dependency systems like requireJS
UMD - Can't think of a good use case where you'd use UMD over the previous 3, but adding it for now for completeness. May get removed in later releases

Was having issues with lodash including everything just for isPlainObject so I rewrote the code to no longer need it. In the future if we do need lodash we should use lodash-es (lodash exported as ES modules).

Also changing the package name to be just eventbrite instead of brite-rest.

Fixes #11

kaylie-alexa

looks good, minor comments -- have lib and dist already been added to .gitignore?

kaylie-alexa · 2018-03-29T00:08:32Z

src/request.ts

-    isPlainObject(responseData['error_detail']) &&
-    isPlainObject(responseData['error_detail']['ARGUMENTS_ERROR']);
+    !!(
+        responseData &&


you don't need to check this with typescript right? i assume you'd get a warning otherwise since responseData is required

So I don't need to check the existence of responseData because of TypeScript, but I do need to check the existence of of the sub properties since they are the response from the server. Will fix!

kaylie-alexa · 2018-03-29T00:17:38Z

package.json

+        "prebuild:dist": "rm -rf dist",
+        "prebuild:lib:cjs": "rm -rf lib/cjs",
+        "prebuild:lib:esm": "rm -rf lib/esm",
+        "prebuild:lib:umd": "rm -rf lib/umd",


I'm tempted to say that we should combine all the lib into one build:lib -- what would be the use case of wanting to build in one lib format but not the other? The time to build would be relatively incremental

Yeah you're probably right. I'll think on it. I don't like the script explosion, but they're definitely gonna be separate in gulp. The kinda sucky part is that the declarations are also put in lib/ so I can't just delete the whole lib/ folder

kaylie-alexa · 2018-03-29T00:19:01Z

gulpfile.js

+    // do the appropriate babel transpile (this is a copy from package.json)
+    .pipe(babel(_getBabelConfig(format)));
+
+const _genUmd = (minify = false) =>


could we do ({minify = false}) instead? Boolean flags make it hard to follow below

kaylie-alexa · 2018-03-29T00:20:18Z

gulpfile.js

+    .pipe(
+      replace(
+        "process.env.NODE_ENV",
+        JSON.stringify(minify ? "production" : "development")


actually instead you could set the parameter to environment = 'development' and then do process.env.NODE_ENV || environment right?

Whoops! I totally forgot to add the conditional minification!

.pipe(uglify()) .pipe(rename({ extname: ".min.js" }))

I know that's not what you meant, but thanks for catching! :)

benmvp · 2018-03-29T01:42:24Z

@kaylieEB yes lib/ & dist/ were already in .gitignore: https://github.com/eventbrite/eventbrite-sdk-javascript/blob/master/.gitignore#L49-L51

- Add missing minified UMD - Remove unnecessary check from `hasArgumentsError` - Clean up build NPM scripts to just have one since we'll likely only call one and we can still call `gulp` directly if needed

kwelch · 2018-03-29T16:18:53Z

I have little gulp and no rollup experience. I will continue to peak at it throughout the day.

kaylie-alexa · 2018-03-29T16:37:15Z

gulpfile.js

+
+      // Minify the code if that option is specified. `null` will get filtered out
+      // below
+      minify ? rollupUglify() : null


can you do minify && rollupUglify()?

Seems like that would work...

benmvp · 2018-03-29T16:47:47Z

@kwelch I'll probably merge the PR but it'll still be here! 😄

kwelch-eb · 2018-03-29T16:48:55Z

@benmvp I see no reason not to merge, I mostly want to learn more about it. I would love to hear why gulp? I would have expected rollup to handle all of these cases.

benmvp · 2018-03-29T16:54:08Z

@kwelch from my investigations Rollup is much like Webpack in that it's really good at taking all of the dependencies and creating a single bundle. Not so good at transpiling and generating target module formats for individual files, like we want for ESM, CJS & UMD.

That's where Gulp comes in. It's much better at taking a bunch of files and processing them all sorts of ways and outputting new versions of them in an efficient/parallel way.

So I guess when you break it down, Rollup is like reduce and Gulp is like map. 😂

rwholey-eb · 2018-03-29T17:09:47Z

package.json

@@ -1,13 +1,13 @@
 {
-    "name": "brite-rest",
+    "name": "eventbrite",


why not something like eventbrite-sdk-javascript

Barrett wanted eventbrite. Having javascript in the name is pretty redundant considering it's a node module. And sdk is pretty much implied?

I thought brite-rest was pretty funny. that being said my first attempt to import it I tried it as eventbrite

rwholey-eb · 2018-03-29T17:13:38Z

gulpfile.js

+];
+
+const _getBabelConfig = format => ({
+  babelrc: false,


is there too much dev stuff in there that we couldnt reuse it here?

Yeah I think there's some sort of bug inheriting it here in gulp. I was trying to override the dev stuff and it just wasn't working, particularly the modules: false which we need for ESM

rwholey-eb · 2018-03-29T17:15:30Z

gulpfile.js

+
+      // Seamless integration between Rollup and Babel
+      rollupBabel(
+        Object.assign(


Cuz this is just running in Node, so didn't wanna assume Node > 8.X. Ideally folks can run the build on Node 6 as well. See: https://github.com/eventbrite/eventbrite-sdk-javascript/blob/master/.travis.yml#L2-L5

kwelch-eb · 2018-03-29T17:51:04Z

Thanks @benmvp! That is an amazing explanation.

ebtravis · 2018-06-22T22:20:52Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

benmvp added 2 commits March 28, 2018 09:40

First pass supporting dist bundle

260acf9

Add umd, esm & cjs build targets

70f51e2

BenAtEventbrite requested review from kaylie-alexa, rwholey-eb and kwelch-eb March 28, 2018 22:01

kaylie-alexa reviewed Mar 29, 2018

View reviewed changes

benmvp added 2 commits March 29, 2018 08:50

Add fixes from @kaylieEB

59a9027

- Add missing minified UMD - Remove unnecessary check from `hasArgumentsError` - Clean up build NPM scripts to just have one since we'll likely only call one and we can still call `gulp` directly if needed

Remove @types/lodash

825bb58

kaylie-alexa reviewed Mar 29, 2018

View reviewed changes

kaylie-alexa approved these changes Mar 29, 2018

View reviewed changes

'Nother suggested @kaylieEB fix

2f7f031

rwholey-eb approved these changes Mar 29, 2018

View reviewed changes

kwelch-eb approved these changes Mar 29, 2018

View reviewed changes

Run build script in Travis

f564358

BenAtEventbrite merged commit 6250f55 into eventbrite:master Mar 29, 2018

benmvp mentioned this pull request Mar 30, 2018

Set up semantic-release for Travis #17

Closed

BenAtEventbrite added this to the Alpha milestone Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(build): Add esm, cjs, umd & dist build targets #14

chore(build): Add esm, cjs, umd & dist build targets #14

benmvp commented Mar 28, 2018 •

edited

Loading

kaylie-alexa left a comment

kaylie-alexa Mar 29, 2018

benmvp Mar 29, 2018

kaylie-alexa Mar 29, 2018

benmvp Mar 29, 2018

kaylie-alexa Mar 29, 2018

benmvp Mar 29, 2018

kaylie-alexa Mar 29, 2018

benmvp Mar 29, 2018

benmvp commented Mar 29, 2018

kwelch commented Mar 29, 2018

kaylie-alexa Mar 29, 2018

benmvp Mar 29, 2018

benmvp commented Mar 29, 2018

kwelch-eb commented Mar 29, 2018

benmvp commented Mar 29, 2018

rwholey-eb Mar 29, 2018

benmvp Mar 29, 2018

kwelch-eb Mar 29, 2018

rwholey-eb Mar 29, 2018

benmvp Mar 29, 2018

rwholey-eb Mar 29, 2018

benmvp Mar 29, 2018

kwelch-eb commented Mar 29, 2018

ebtravis commented Jun 22, 2018

chore(build): Add esm, cjs, umd & dist build targets #14

chore(build): Add esm, cjs, umd & dist build targets #14

Conversation

benmvp commented Mar 28, 2018 • edited Loading

kaylie-alexa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmvp commented Mar 29, 2018

kwelch commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmvp commented Mar 29, 2018

kwelch-eb commented Mar 29, 2018

benmvp commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwelch-eb commented Mar 29, 2018

ebtravis commented Jun 22, 2018

benmvp commented Mar 28, 2018 •

edited

Loading