Add bundler scripts for stream #1832

spacejack · 2017-05-01T23:13:12Z

Following up on #1831

Took a stab at adding bundler scripts for stream, so consider this a rough draft. :) I'm not sure this won't break anything... but the tests still run ok.

This removes the old top-level stub stream.js and replaces it with a bundled output (analogous to mithril.js and mithril.min.js.)

Note that I also changed the build script from:

npm run build-browser & npm run build-min

to:

npm run build-browser && npm run build-min

IMHO I would add the npm-run-all module so that the dev scripts could become:

"dev-mithril": "node bundler/cli browser.js -o mithril.js -w",
"dev-stream": "node bundler/cli stream/stream.js -o stream.js -w",
"dev": "npm-run-all -p dev-mithril dev-stream",

dead-claudia

Beyond the two nits, make sure that the bundler doesn't update the README using the stream bundle's size - Mithril core is a bit larger than the ~1.5kb gzipped, but unminified stream core. This may require an extra flag.

dead-claudia · 2017-05-01T23:26:44Z

package.json

@@ -21,7 +22,7 @@
    "cover": "istanbul cover --print both ospec/bin/ospec",
    "release": "npm version -m 'v%s'",
    "preversion": "npm run test",
-    "version": "npm run build && git add mithril.js mithril.min.js",
+    "version": "npm run build && git add mithril.js mithril.min.js stream.js stream.min.js",


Nit: use mithril-stream.js and mithril-stream.min.js instead, so it doesn't override mithril/stream.

dead-claudia · 2017-05-01T23:27:30Z

.eslintignore

@@ -4,4 +4,6 @@
 /examples
 /mithril.js
 /mithril.min.js
+/stream.js


Nit: use mithril-stream.js and mithril-stream.min.js instead, so it doesn't override mithril/stream.

package.json main points to mithril.js. So when doing require/import we'd get mithril.js, not index.js, right? So would we not also want to get the bundled, top-level stream.js?

And when using unpkg.com in a jsbin I was thinking it'd be nice to be able to use unpkg.com/mithril/stream rather than unpkg.com/mithril/mithril-stream

When you require("mithril/stream"), it searches for the following files (source):

node_modules/mithril/stream.js

node_modules/mithril/stream.node (and other registered extensions)

node_modules/mithril/stream/{pkg.main} (where "main" is from the package.json)

node_modules/mithril/stream/index.js

node_modules/mithril/stream/index.node (and other registered extensions)

This means when you save a file as stream.js in the root, when it gets published, Node will find mithril/stream.js rather than the intended mithril/stream/index.js.

@spacejack (quoting from your most recent reply)

Currently, using require/import you get the pre-bundled mithril.js. My attempt was to create a similar stream.js bundle.

Oh okay. That makes sense (although that begs the question of why it's still in a subdirectory - the main export is an unbundled UMD).

But for the purposes of this review, my nits are moot.

tivac · 2017-05-01T23:40:48Z

https://www.npmjs.com/package/mithril-stream is published from the /stream dir, not the root, so these files won't get correctly published to npm.

I'd like to see those bundle steps broken out further so the command length stays short enough to be readable.

This is starting to feel just creaky enough that it maybe should lead to some reworking of the bundling stuff in general. Especially in regards to ospec and mithril-stream and how to handle those.

spacejack · 2017-05-01T23:45:33Z

This might get beyond the scope of what I can or should be doing, so feel free to re-do the PR. :)

dead-claudia · 2017-05-01T23:46:46Z

Or better, maybe factor this out into a Node script? We're getting to the point ShellJS is starting to seem attractive IMHO. Easier to maintain, and we can do more. Plus, it remains platform-independent, which shell scripts and npm scripts are not.

dead-claudia · 2017-05-01T23:47:48Z

An added bonus running for ShellJS is that it comes with this useful ultra-basic task runner in the form of shelljs/make, which you just require at the top of the entry script. (No special CLI for it.)

spacejack · 2017-05-01T23:52:20Z

Personally I don't mind lots of short, atomic npm scripts and then I use npm-run-all to combine them. In sequence or in parallel as needed.

tivac · 2017-05-02T00:11:45Z

I prefer npm scripts where possible, with a smattering of custom build scripts where necessary. ShellJS is a great tool for those custom build scripts for sure.

I don't really have the time for a big rework right now, so this sort of change is probably fine so long as it puts the files in the correct directory.

dead-claudia · 2017-05-02T00:13:12Z

@tivac

I don't really have the time for a big rework right now, so this sort of change is probably fine so long as it puts the files in the correct directory.

I know. I wasn't trying to block this PR beyond that of my review.

spacejack · 2017-05-02T00:28:41Z

To publish /stream (for the npm module mithril-stream) we'd have to move the stream.js source file elsewhere. (Would need to track down any references to the stream source file in the repo.)

IMHO It would be easier to deprecate the mithril-stream lib and only publish mithril/stream. This way require/import and jsbins using unpkg, rawgit, etc. all use predictably similar paths.

import m from 'mithril'
var stream = require('mithril/stream')
<script src="https://unpkg.com/mithril">
<script src="http://rawgit.com/lhorie/mithril.js/next/stream.js">

Would also need an answer for #1832 (comment) before I can proceed.

spacejack · 2017-05-02T00:33:20Z

I guess one other option would be to leave stream/stream.js where it is and bundle to stream/index.js. Then update stream/package.json: "main": "index.js". And remove /stream.js

But this is a little different than how the top level package.json main points to mithril.js.

spacejack · 2017-05-26T19:05:42Z

@tivac @isiahmeadows Just bumping this PR thread.

To review - I'm uncertain what the desired layout and what files should be consumed when a user bundles mithril & stream.

Currently, using require/import you get the pre-bundled mithril.js. My attempt was to create a similar stream.js bundle. (See #1832 (comment))

Was also hoping to provide nice paths for usage in jsbins via unpkg.com.

If I have some clarity on the desired file layout I could get back on this.

dead-claudia · 2017-05-27T02:59:47Z

I approved as my concerns are now moot.

spacejack · 2017-05-27T20:43:20Z

I've updated the bundler cli script to only update the readme when building mithril.min.js (not stream.min.js.)

That said, was doing some testing before that and noticed that the size wasn't being updated anyway. I think the regex isn't matching.

dead-claudia · 2017-05-28T02:20:29Z

@spacejack If you could fix the regexp problem (could be in either one) in this PR, that'd be great.

@tivac Any other blockers to merging the PR itself? So far, I don't see any. (We can still continue discussion, though.)

spacejack · 2017-05-28T16:21:45Z

Ok, the regex now updates the reported gzip sizes in the readme. Minor edit so it could update both places.

tivac · 2017-07-13T03:40:33Z

Sorry @spacejack, I stomped all over this with #1898. I think we should fix the stream stuff in a more holistic way (#1875), but I'll cut new mithril-stream and ospec releases in a minute here so at least they aren't so out of date

StephanHoyer · 2022-06-06T11:00:48Z

will be fixed with #2753

Add bundler scripts for stream

65756c7

dead-claudia requested changes May 1, 2017

View reviewed changes

dead-claudia previously approved these changes May 27, 2017

View reviewed changes

spacejack added 2 commits May 27, 2017 16:08

Merge branch 'next' into stream-bundle

8f750bd

Only update readme size when minifying mithril.min.js

971a6e0

Use dblquotes for linter

916238c

Fix regex to replace current gzip sizes in readme

b208db5

Fix regex to match sizes >= 10kb

0e2551e

tivac added the Area: Workflow For anything dealing with Mithril's internal tooling, including the mocks and bundler, but not ospec label Jul 20, 2017

dead-claudia force-pushed the next branch from 292a667 to 51e0aee Compare May 29, 2019 14:11

dead-claudia dismissed their stale review via 0e2551e July 31, 2021 06:04

StephanHoyer mentioned this pull request Jun 6, 2022

[WIP] Kick bundler - use esbuild for building #2753

Closed

3 tasks

StephanHoyer closed this Jun 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bundler scripts for stream #1832

Add bundler scripts for stream #1832

spacejack commented May 1, 2017 •

edited

Loading

dead-claudia left a comment

dead-claudia May 1, 2017

dead-claudia May 1, 2017

spacejack May 1, 2017

dead-claudia May 27, 2017

dead-claudia May 27, 2017 •

edited

Loading

tivac commented May 1, 2017

spacejack commented May 1, 2017

dead-claudia commented May 1, 2017

dead-claudia commented May 1, 2017

spacejack commented May 1, 2017 •

edited

Loading

tivac commented May 2, 2017

dead-claudia commented May 2, 2017

spacejack commented May 2, 2017 •

edited

Loading

spacejack commented May 2, 2017

spacejack commented May 26, 2017

dead-claudia commented May 27, 2017

spacejack commented May 27, 2017

dead-claudia commented May 28, 2017

spacejack commented May 28, 2017

tivac commented Jul 13, 2017

StephanHoyer commented Jun 6, 2022

Add bundler scripts for stream #1832

Add bundler scripts for stream #1832

Conversation

spacejack commented May 1, 2017 • edited Loading

dead-claudia left a comment

Choose a reason for hiding this comment

dead-claudia May 1, 2017

Choose a reason for hiding this comment

dead-claudia May 1, 2017

Choose a reason for hiding this comment

spacejack May 1, 2017

Choose a reason for hiding this comment

dead-claudia May 27, 2017

Choose a reason for hiding this comment

dead-claudia May 27, 2017 • edited Loading

Choose a reason for hiding this comment

tivac commented May 1, 2017

spacejack commented May 1, 2017

dead-claudia commented May 1, 2017

dead-claudia commented May 1, 2017

spacejack commented May 1, 2017 • edited Loading

tivac commented May 2, 2017

dead-claudia commented May 2, 2017

spacejack commented May 2, 2017 • edited Loading

spacejack commented May 2, 2017

spacejack commented May 26, 2017

dead-claudia commented May 27, 2017

spacejack commented May 27, 2017

dead-claudia commented May 28, 2017

spacejack commented May 28, 2017

tivac commented Jul 13, 2017

StephanHoyer commented Jun 6, 2022

spacejack commented May 1, 2017 •

edited

Loading

dead-claudia May 27, 2017 •

edited

Loading

spacejack commented May 1, 2017 •

edited

Loading

spacejack commented May 2, 2017 •

edited

Loading