Skip to content
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

Further reduce size #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Further reduce size #1

wants to merge 3 commits into from

Conversation

developit
Copy link

Hi Josh!

I really like this little library. I know you're partial to Browserify, but I figured I'd try running it through Microbundle to see if there was much of a size difference. I ended up getting it down to 166 bytes, which seems pretty good:

screen shot 2018-09-20 at 4 35 40 pm

Totally up to you whether this seems like a build you could live with, just figured I'd send over the results since the size drop was >50%.

Cheers!

@TehShrike
Copy link
Owner

Thanks, I appreciate it!

Nice to "internet meet" you, I think I started following you on Github in 2015 😂

I actually do use Rollup instead of Browserify for bundling nowadays, I only really use browserify in cases like this where I just need to pipe a bundle into a CLI tool for testing, like tape-run.

I love the idea of microbundle, I hadn't run into it before!

Were you comparing the microbundle output to browserify output?

I'm cool with publishing minified output, but I have taken to publishing ES2015 instead of ES5 in the last year, assuming that anyone who still targets IE11 is using babel+preset-env. Should save some bytes there.

I'll take a better look at this in a day or two.

@@ -0,0 +1,27 @@
export default workFn => {
const hasIdleCallback = typeof requestIdleCallback==='function'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is more sensible than the window/global boilerplate I pasted in originally.

@@ -8,7 +7,7 @@ const delay = require('delay')
const nextIdleCallback = () => {
const deferred = makeDeferred()

idleish(deferred.resolve, 2000)
setTimeout(deferred.resolve, 2000);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was roughly "when testing in the browser, I do want to assert that events happened before the next idle callback". Arguably not worth the extra test complexity though.

If it's not worth it to keep the idle-callback-based assertion, we could just drop nextIdleCallback and replace it in the code with await delay(2000)

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

Successfully merging this pull request may close these issues.

None yet

2 participants