Include source code in published activestorage npm package

[rmacklin]

Summary

By including the source code in the published npm package, we enable activestorage users to ship smaller javascript bundles to visitors using modern browsers. (And it seems like there is already support from rails core members for doing precisely that: rails/webpacker#887 (comment).)

I've demonstrated this in this sample repository:
https://github.com/rmacklin/activestorage-es2015-build-example

In that example, the bundle shrinks by 5K (24%).

In addition to allowing smaller bundles for those who ship untranspiled code to modern browsers, including the source code in the published package can be useful in other ways:

1. Users can import individual modules rather than the whole library
2. As a result of (1), users can also monkey patch individual parts of activestorage by importing the relevant module, modifying the exported object, and then importing the rest of activestorage (which would then use the patched object).

Other Information

In order to allow the source code to be depended on rather than the compiled code, we have to declare the external dependency on spark-md5 as a regular dependency, not a development dependency.

This means that even users who depend on the compiled code will have to download this package. However, spark-md5 is a small package (and may already be downloaded as a transitive dependency of another package), so this tradeoff seems worth it.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[rails-bot]

Warning

• Pull requests are usually filed against the master branch for this repo, but this one is against 5-2-stable. Please double check that you specified the right target!

[rmacklin]

I did intend to target this to 5-2-stable because 5.2 (and activestorage) is not yet released and I think we should get this into the official 5.2 release. If this change is accepted, it will be easy to pull it into master (I'd be happy to open a separate PR for that, though I'm assuming all changes that make it into the official 5.2 release will be merged into master).

[rmacklin]

I'm not sure how @rails-bot picks reviewers, but @pixeltrix does not have any commits in activestorage, so I think @javan (who is specified as the npm package's author) may be a more appropriate reviewer.

[rmacklin]

The Travis failure is:

Failure:
NestedThroughAssociationsTest#test_has_many_through_has_one_with_has_one_through_source_reflection_preload_via_joins [/home/travis/build/rails/rails/activerecord/test/cases/associations/nested_through_associations_test.rb:124]:
2 instead of 1 queries were executed.

only on ruby 2.2.8, so it seems unrelated (my changes do not touch activerecord, only activestorage, and they don't touch ruby code)

[javan]

I'm OK with publishing the source files in the npm package, but I'd prefer to see them copied to a more convenient git-ignored location when releasing so you don't have to import activestorage/app/javascript/activestorage or set up an alias.

In that example, the bundle shrinks by 5K (24%).

That's because you're compiling (less verbose) ES2015 instead of ES5, right?

I did intend to target this to 5-2-stable because 5.2 (and activestorage) is not yet released and I think we should get this into the official 5.2 release

Please file this against master and we'll backport as neccessary.

[rmacklin]

Please file this against master and we'll backport as neccessary.

Ok, sorry about that! I've rebased off master and updated the PR target branch. I left in the update to the changelog, but if this is going to be backported to 5.2, should I remove that commit and let the backport PR handle it?

[rmacklin]

That's because you're compiling (less verbose) ES2015 instead of ES5, right?

Exactly. Module authors are encouraged to publish untranspiled code now because it allows developers to serve the untranspiled code to supporting browsers (which, these days, covers the majority of visitors). Not only do we get the benefits of reduced download and parse time, but now that we're shipping the native syntax (instead of transpiling it), the JS engines can apply optimizations for the native syntax.

[rmacklin]

I'm OK with publishing the source files in the npm package, but I'd prefer to see them copied to a more convenient git-ignored location when releasing so you don't have to import activestorage/app/javascript/activestorage or set up an alias.

Sounds good. I've pushed 9f9f77ec5600daccb642dc0c1df5cf4d1be6a536 which copies the files to a git-ignored src directory. Now the transpiled code can be imported from activestorage, and the source code can be imported from activestorage/src. Let me know if you'd prefer a different folder name - I just picked src because that's what's used in other packages I've been importing the source code from.

[javan]

I think this should use prepublishOnly instead of prepublish.

• prepublish: Run BEFORE the package is packed and published, as well as on local npm install without any arguments.
• prepublishOnly: Run BEFORE the package is prepared and packed, ONLY on npm publish.

https://docs.npmjs.com/misc/scripts

[rmacklin]

Got it - changed to prepublishOnly in 266f8b0021d6b7258bceaa4a3434e66746c14638

[javan]

Let's make sure src/ doesn't have any stale files in it: rm -rf src && cp -R app/javascript/activestorage src

[rmacklin]

Good idea. Addressed in 0e89643a7038c8b4d4e1aad0efa8ef759473aa06

[javan]

Looks great! Please squash your commits and then we're good to go.

[rmacklin]

Commits have been squashed and the branch has been rebased off the head of master.

[javan]

Thank you!

[rmacklin]

Thanks very much for your help!

As far as getting this change into the official activestorage release in rails 5.2.0, what's the process around that? Anything I can help with?

[javan]

Being more of an enhancement / feature than a bug fix, it may not be released until 5.2.1. It seems harmless enough to add to 5.2.0 though since it doesn't make any actual code changes. What do you think, @rafaelfranca?

[rafaelfranca]

I think it is fine to include. Just backported

[rmacklin]

Awesome, thanks again for your help (and for your work on activestorage & the rest of rails)!