Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Create flat bundles & ship module entry #78

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

Andarist
Copy link
Contributor

At the moment I've done my changes (besides dep updates) only in react-redux-subspace. I'd like to check first if you like this and if you want to include the change, if yes then I will make the same changes to other packages (they are pretty much copy-pasteable).

What this gives?

  • distributing secondary module entry in ESM format which allows tools such (such as webpack or rollup) optimize consumers' bundles with tree-shaking & scope hoisting
  • flat bundles - hiding internal directory structure of the package, this also reduce overall size of the consumers' bundles, because things such as babel helpers are deduplicated within the package & it's easier to dead code eliminate (aka tree-shake) when things are contained to a single javascript scope

You could also incorporate using my babel plugin - https://github.com/Andarist/babel-plugin-annotate-pure-calls - to let things created at runtime be tree-shaken (if not used ofc). I can tell more about what it gives if you are interested, just let me know.

@mpeyper
Copy link
Contributor

mpeyper commented Feb 10, 2018

It's like you read my mind. I had a note to add an issue for this next time I got to the repo.

I've been keen on the idea since I read the react blog post and my interest was piqued again when I noticed redux shipping es modules, so this is very much something I'd like for redux-subspace.

In that respect, I'm a big fan of "doing what the others do", so having roll-up configs and outputs as close as possible to something like redux is preferably.

I'd like to hear more about your plugin, as we do a lot of creating things at runtime.

Having said all this, I won't have time to look at this properly for a few days. Feel free to push forward with it until then, but keep that in mind.

Remember to add yourself to the contributors list before you're done too.

@Andarist
Copy link
Contributor Author

The story behind my plugin is quite simple. At the moment it's uglifyJS which removes dead code when using webpack, webpack "only" removes assignments to exports object (but that allows for dead code removal later). Side note - upcoming webpack@4 is going to remove at least some cases on its own, i've seen it working - just havent explored what exact cases webpack can remove.

Problem with dead code elimination in javascript lies in its highly dynamic nature, tools are "afraid" to remove too much because every removal is a potential runtime risk. Some cases are trivial - such as if (false) ..., some are not.

It's also easy to remove i.e. function declarations - because if it's not referenced anywhere, it is not used. It's also easy to remove a variable assignment if the variable is not used. Let's examine a sample code:

var unused = someFunctionCall()

unused can be safely eliminated, but tools such as uglifyJS have no idea what someFunctionCall does and if it does not produce any meaningful side effects when called. So the only reasonable thing to do is to leave the call and ofc the called function too (and whatever it references) untouched.

But as authors we know what our code is supposed to do and we can give hints for those tools by annotating function calls with #__PURE__ comment - this is a sign for UglifyJS that the annotated call can be safely removed if its output stays unused.

The function itself doesnt really even have to be pure (considering what pure actually means), it only cant have what I call "meaningful side effects", actually most of properly structured code shouldnt have any non-meaningful side effects associated with it. More about this rationale can be read here.

So to sum it up - my plugin just automates this task of annotating those function calls that have a potential of being removed (only calls that get executed during initialization of the code).

@mpeyper
Copy link
Contributor

mpeyper commented Feb 10, 2018

That sounds pretty good. Just one question:

my plugin just automates this task of annotating those function calls that have a potential of being removed

How does it tell which functions have that potential?

@Andarist
Copy link
Contributor Author

I assume every function is pure or only with "meaningful" side effects and I annotate every function call which result is being used in "assignment context", this means that I wont annotate:

sideEffectful()

but I will annotate both in

var result = pureCall(withMeaningfulSideEffect())

Only those calls that gets executed during initialization can be removed, so I dont annotating anything in such code:

function someFn() {
  var result = wontAnnotate()
}

@mpeyper
Copy link
Contributor

mpeyper commented Feb 10, 2018

Cool. That sounds perfect for this project.

@Andarist
Copy link
Contributor Author

Cool, let's keep this PR focused though - I can always create a followup PR later.

I've refactored all packages, most of the changes are the same, but in 1 or 2 places packages had slightly different setup. This also should get like a next prerelease or something which you could test out first before releasing it to the audience.

While I'm pretty confident that I know what I'm doing those things are unfortunately hard to test. I hope I haven't missed anything.

@Andarist Andarist changed the title [wip] Create flat bundles & ship module entry Create flat bundles & ship module entry Feb 10, 2018
@@ -9,13 +9,16 @@
],
"license": "BSD-3-Clause",
"main": "lib/index.js",
"typings": "lib/index.d.ts",
"module": "lib/index.es.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any advantage to moving the the es module into a es directory?

e.g.

"module": "es/index.js"

Then someone could force the use of the es module like so:

import { SubspaceProvider } from 'redux-subspace/es'

At least, I think that's how it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it would work like that (same can be done now, but with more verbose - redux-subspace/lib/index.es.js)

I dont know why anyone would like to force this, tools should be responsible for making the best choice based on their capabilities

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no difference in it then I'd prefer the ex/index.js approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I've adjusted the PR

@@ -9,13 +9,16 @@
],
"license": "BSD-3-Clause",
"main": "lib/index.js",
"typings": "lib/index.d.ts",
"module": "lib/index.es.js",
"typings": "index.d.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I really liked having the type definition next to index.js in the src directory instead of the package root. I get that we aren't copying it to the lib directory anymore with babel, but is there an alternative we can use? Perhaps referencing it directly from the src directory or copying it manually as a seperate command?

Not a deal breaker for this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just omitted the need to have additional script, it's certainly doable though - would prefer leaving this issue for a followup PR, but can do that some time later as part of this one too. Just tell me which one do u prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it as you have it for now. Not having it in the root is my personal preference, but not very common amongst most libraries I've seen.

'stage-3',
],
plugins: [
cjs && 'transform-es2015-modules-commonjs',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be loose too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've recently started to omit loose of cjs transform because I feel the gain is minimal. The only thing it does is that it makes "magic" __esModule property enumerable (it's simply assigned and not defined with defineProperty) and that makes spreading namespaces weird, as in

import * as namespace from './namespace'
const obj = { ...namespace } // oops, obj has `__esModule` property

cjs && 'transform-es2015-modules-commonjs',
['transform-runtime', {
helpers: false,
polyfill: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Some funny whitespacing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

{
"presets": ["env", "react", "stage-3"]
}
{ "presets": ["./.babelrc.js"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a common approach? I haven't seen many projects with anything other than a babelrc file and an env node for variations (although they do require multiple build scripts to seperate them).

Are there any benefits one way or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an useful hack to construct ur configs with javascript instead of plain JSON. Upcoming babel@7 will have support for .babelrc.js by default so you will be able to remove this "proxy" .babelrc file

I was always a little bit confused about how merging is done when specifying env in .babelrc - and this had to memorized and checked each time, when with .babelrc.js I just know instantly what's the output config based on javascript conditions, without anything being automated (sometimes unintuitively for me) by babel itself

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mpeyper
Copy link
Contributor

mpeyper commented Feb 12, 2018

Left a few comments (mostly questions as I'm a bit unfamiliar with this).

Overall, very happy with this.

@Andarist Andarist force-pushed the module-entries branch 2 times, most recently from 8c70a62 to d597aaa Compare February 12, 2018 22:31
@mpeyper
Copy link
Contributor

mpeyper commented Feb 13, 2018

Hey @Andarist, can you also add yourself to the contributor list under the packaging category, and then I'll merge this ASAP.

@Andarist
Copy link
Contributor Author

Done

@mpeyper mpeyper merged commit ac1ff20 into ioof-holdings:master Feb 13, 2018
@Andarist Andarist deleted the module-entries branch February 13, 2018 11:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants