-
Notifications
You must be signed in to change notification settings - Fork 271
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
Run browserify on NPM prepublish #52
Conversation
@@ -34,6 +34,7 @@ | |||
"coveralls": "npm run cov && coveralls < ./coverage/lcov.info", | |||
"build-min": "browserify src/index.js -s geojsonvt | uglifyjs -c -m -o geojson-vt.js", | |||
"build-dev": "browserify -d src/index.js -s geojsonvt -o geojson-vt-dev.js", | |||
"watch": "watchify -v -d src/index.js -s geojsonvt -o geojson-vt-dev.js" | |||
"watch": "watchify -v -d src/index.js -s geojsonvt -o geojson-vt-dev.js", | |||
"prepublish": "browserify -d src/index.js -s geojsonvt -o geojson-vt-dev.js; browserify src/index.js -s geojsonvt | uglifyjs -c -m -o geojson-vt.js" |
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.
let's compose other npm scripts (e.g. npm run build-min
) rather than copypasting
Also, if we want the build files to get into NPM, we have to add |
There is already a |
@IvanSanchez nope, there's only |
Oh for the noodles of the FSM, I'm looking inside |
7ccbf98
to
41a7ef6
Compare
@mourner I hope the updated commit should do the trick. |
@@ -14,7 +14,7 @@ | |||
"type": "git", | |||
"url": "git://github.com/mapbox/geojson-vt.git" | |||
}, | |||
"main": "src/index.js", | |||
"main": "geojson-vt-dev.js", |
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.
Why change the main entry point?
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.
Because I'd like to have the bundled file ready for loading it up in a web browser. On second thought, we could use the browser
field of package.json
as explained here.
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.
Why? If you're using browserify or rollup or etc, it'll handle the main
file just fine. I don't see any reason to include browser
or change main
of this project. Running browserify
on prepublish will be nice if we want an npmcdn
workflow, but it should be tightly scoped to that need, not change essential stuff like the main
entry
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.
Yeah, main should remain src/index.js
.
Merged with some tweaks as 12eb850 |
Kinda bringing #24 back from the dead (I expect browser builds after running
npm install geojson-vt
), but implemented in the same nice way as Leaflet/Leaflet#1573