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.

Sign up for GitHub

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

Jump to bottom

Export version property on base object #25

Merged
merged 5 commits into from
Sep 19, 2015
Merged

Export version property on base object #25

merged 5 commits into from
Sep 19, 2015

Conversation

jasonkarns
Copy link
Member

No description provided.

@searls
Copy link
Member

searls commented Sep 14, 2015

👎 on uglify for the main dep. I'm fine with an uglify second dep, but
since it's a test lib I'd rather leave it unminified in my own use

  • Lets not commit to dist/ except when running npm version so only
    released distributions ever hit git.
  • can we get the version at the top of the built file in a comment? (e.g.
    /* testdouble@0.2.0 - https://github.com/testdouble/testdouble.js */)
    On Sun, Sep 13, 2015 at 20:40 Jason Karns notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

#25
Commit Summary

  • add uglifyify
  • add version to root object
  • grab version from env

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#25.

@jasonkarns
Copy link
Member Author

on uglify for the main dep.

uglify is required to prune package.json out of the bundle. we need its unreachable-branch cleanup to keep package.json from being included from https://github.com/testdouble/testdouble.js/pull/25/files#diff-4ef89470f29f8c6989c09e54066b4623R7

Lets not commit to dist/

I don't normally insert the built versions but did so intentionally in this branch so that we can see the uglified-without-version-property build and diff it with the uglified-with-version-property build. Can remove if still desired.

version at the top of the built file in a comment?

we probably could simply by cating the string into the file, but browserify doesn't have any facilities for inserting comments. i only ever look for version strings from the browser, not the file itself. re-use uglify (proper) with the banner option?

@searls
Copy link
Member

searls commented Sep 14, 2015

uglify is required to prune package.json out of the bundle. we need its unreachable-branch cleanup to keep package.json from being included from https://github.com/testdouble/testdouble.js/pull/25/files#diff-4ef89470f29f8c6989c09e54066b4623R7

Regardless, I don't want to minify it by default (I don't care if the package.json is in the built asset, but I also don't care if testdouble.version is defined and would rather exclude it entirely than have to minify it)

Can remove if still desired.

Yes I only want to have anything generated to dist or committed when I run npm version so that master is always guaranteed to point at a real release and I don't have to remember to reset it manually otherwise.

we probably could simply by cating the string into the file,

That or any other means is fine. Bizarre to me that there's not a browserify functionality for this. Is publishing browser-consumed libraries not a common use case for browserify?

@jasonkarns
Copy link
Member Author

Bizarre to me that there's not a browserify functionality for this. Is publishing browser-consumed libraries not a common use case for browserify?

Browser-consumed libraries are almost entirely consumed in minified/uglified form. (not to mention how impossible they are to read with browserify scaffolding) I think for that reason I'm seeing less and less use of comments for versioning and more use of the version property on the exported object instead. anecdotal

I don't care if the package.json is in the built asset

If this is the case, then we can kill uglifyify and envify both, and just export require('./package.json').version. Much easier; only downside is having the package.json contents itself in the bundle. (there's actually a separate transform that's meant to do dead-code removal without the uglification part; which we could use in place of uglifyfy. except it's broken and doesn't actually work. huzzah)

@jasonkarns
Copy link
Member Author

removed all the dist builds and removed uglifyify/envify. so package.json contents are currently being included in the bundle. we can fix this without using uglifyify when zertosh/unreachable-branch-transform#9 is fixed by using envify + unreachable-branch-transform

@searls
Copy link
Member

searls commented Sep 14, 2015

well this PR got a lot smaller.

i therefore demand we write a test for this since our coverage is pretty fab right now

@jasonkarns
add version to root object
e57cdbd 
as it gets the version from package.json (via require), the entire
package.json contents end up in the bundle
@jasonkarns
test the version property exists
ace586c
@jasonkarns
grab version from npm env if it's there
15b70f2
@jasonkarns
add envify transform
290a6fd 
the envify transform will replace any `process.env` usage with the
static values at build time. If the env variable isn't set, nothing
will be substituted so it will still pick up the runtime value.

This means that any build-time env vars can be statically added to the
build without browserify needing to include the browserify-core to get
`process` at runtime.

Specifically for us, we're grabbing the version out of
npm_package_version so that we can skip the package.json require
@jasonkarns
Add unreachable-branch-transform
672a82f 
This transform eliminates dead-code branches.

Specifically, we use this so that once the npm_package_version variable
has been substituted with a static value (via envify), the || conditional is now
satisfied so the package.json require becomes dead code. This transform
removes the require so browserify doesn't need to include package.json
in the bundle.
@jasonkarns
Copy link
Member Author

@searls rebased to master. test added. put envify back in now that unreachable-branch-transform is usable in place of uglifyify.

So browser build runs through envify; gets version string from process.env.npm_package_version; runs through unreachable-branch-transform which kills the package.json require; so package.json is omitted from the build.

In node, the version is plucked from the require('package.json').version

@searls
Copy link
Member

searls commented Sep 19, 2015

LGTM

searls added a commit that referenced this pull request Sep 19, 2015
@searls
Merge pull request #25 from testdouble/version
fcb7dfc 
Export `version` property on base object
@searls searls merged commit fcb7dfc into master Sep 19, 2015
@searls
Copy link
Member

searls commented Sep 19, 2015

✨

@searls searls deleted the version branch September 19, 2015 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jasonkarns @searls