-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
--slim by default. Fixes gh-412, Fixes gh-418 #420
Conversation
// If there is only one file in this project, then there is | ||
// no reason to use the code generated by browserify, because | ||
// it will always have the module loading boilerplate included. | ||
if (opts.single || fileCount === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. :)
Great fixes and refactor. 👍 |
@HipsterBrown thanks for the review :D |
}) | ||
.option('slimPath', { | ||
default: 'build.js' | ||
}) | ||
.option('full', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this before. Should this option, and making --slim
the default action, be added to the push
command as well?
.option('full', { | ||
flag: true, | ||
default: false, | ||
help: 'Bundle all files in project' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to make this more explicit: "Bundle all files in project including those not used by the program".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Bundle all files in project including those not used by the program, but excluding any files matched by non-negated rules in .tesselignore"
From @LinusU in another issue:
Thoughts on this approach @rwaldron? |
@HipsterBrown that's out of scope for this changeset. I don't want to pig-pile a bunch of changes into one PR. |
@rwaldron That works for me. What about having a better namespaced path for the generated bundle? |
Yeah, that should be addressed here, update to follow |
3ac6b80
to
41e29ce
Compare
After re-reviewing the conversation, I decided that you and @LinusU were right and this needed to be addressed. The slim path now...
No there is no chance of paving over user project files! Thanks for highlighting this issue :) |
4ab7d60
to
e23853d
Compare
Just to clarify about I think that this is the minimum we should do thought, I'm pro this change. However, what I really wanted was something like this: const source = browserify(...).bundle()
const gzip = zlib.createGzipStream()
source.pipe(gzip).pipe(/* to the tessel */) It's unnecessary to use tar since we are only sending one file over. I also feel it's a bit overkill to create a temporary directory, stream the entire file to disk, use a new stream that iterates directories, then create a tar stream out of that and then pipe that to the tessel. In reality, the only thing that happens is that a few bytes gets prepended to the stream before the file. tar is a very simple format. But as I said, this is absolutely good for now! |
Yep, I was monitoring the actual creation of dirs—easier to think about an ephemeral concept as "in-memory"
This was brought up in another thread and I held off responding because I thought @HipsterBrown's mention of the
Skipping the phase where this occurs means eliminating the opportunity to optimize the resulting bundle; in cases where browserify has only bundled a single file, the original file (minified) will always be a smaller payload because it will have the same contents as the browserify result, but will not have the module loading/dependency boilerplate. Also, the same argument I gave above applies to this phase. Do you know of a way to simplify the operation that preserves the all of these semantics? |
5d89658
to
d1c4a18
Compare
Thanks for the reviews! |
I just realized that the tar header needs to contain the length of the file :( I thought that I was so smart thinking there was a way to just append the header ourselves but that won't work. I can totally see the benefit of having the same receiving mechanism for both a single file and a directory of files. A thing that I would like slightly more, mostly because it skips the const fs = require('fs')
const tar = require('tar-stream')
const temp = require('fs-temp')
const browserify = require('browserify')
const file = temp.createWriteStream()
const source = browserify(...).bundle()
source.pipe(file).on('finish', function () {
const pack = tar.pack()
const infile = fs.createReadStream(file.path)
const header = { name: 'index.js', size: file.bytesWritten }
const entry = pack.entry(header, function (err) {
if (err) throw err
pack.finialize()
})
infile.pipe(entry)
pack.pipe(/* To the Tessel */)
}) I don't know if this feels like extra work, but on the other hand, getting the deployment to go as fast as possible would be very nice. It would be interesting to see if actually yields any difference though. It feels cleaner since it doesn't use |
No description provided.