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

browserify #67

Merged
merged 21 commits into from Nov 5, 2013
Merged

browserify #67

merged 21 commits into from Nov 5, 2013

Conversation

calvinmetcalf
Copy link
Member

I take back more or less everything I said about paths in #48 @sheppard was write especially when it comes to browserify compatibility and speaking of which it turns out that building with browserify instead of with requirejs is like 1000% simpler. What do people think of this? we could even go one farther and just strait up use browserify.

@ahocevar
Copy link
Member

ahocevar commented Oct 1, 2013

I have no objections to this.

@calvinmetcalf
Copy link
Member Author

ok here is the version with no amd modules just strait up commonjs...I swear this will be the last time I totally rearrange the library in some sort of fit of OCD

@sheppard
Copy link
Contributor

sheppard commented Oct 1, 2013

Hmm, I'm obviously sad to see CommonJS source modules instead of AMD, but I haven't had a need to use Proj4js with browserify so I can't comment on the interop issues. I'll keeping holding out hope for an AMD future (at least until ES6).

@sheppard
Copy link
Contributor

sheppard commented Oct 1, 2013

On a minor note, it looks like proj4 got replaced in some comments where it shouldn't have been (e.g. proj4.showError). It's easier to see in b3a9684.

@calvinmetcalf
Copy link
Member Author

The main advantages I found were

  • there are too many files to realistically every load it page by page in a browser, node on the other hand can realistically load all 30 files.
  • Browserify seems to be built to do what we want to do from the start so things like UMD definitions are built in and not some sort of half baked after though.
  • The output is the same but with less boilerplate.

@calvinmetcalf
Copy link
Member Author

Good catch though it's only in comments, I need to remove all those showErrors anyway

@sheppard
Copy link
Contributor

sheppard commented Oct 1, 2013

Yeah, I can see why this approach makes sense, I was just really hoping we could get AMD to work. I noticed jQuery core just made the switch to r.js but they're using some non-standard regex magic to pull out the define()s from the built file. FWIW, I liked the dist/amd/proj4.js version which had the smallest amount of boilerplate - but of course assumed that the consumer would be using an AMD loader. I don't suppose we can amdify to keep that around?

Once ES6 stabilizes it's going solve all our module system disparities, right?

@ahocevar
Copy link
Member

ahocevar commented Oct 1, 2013

Good points on AMD @sheppard. I basically agree, but I don't have the time to work in that direction for now unfortunately. But I don't think it makes sense for the browser to do the loading - too many files as @calvinmetcalf points out correctly.

@calvinmetcalf
Copy link
Member Author

@sheppard normally Id agree about the space savings but in this case for the minified (but not gzipped) code we are talking about a less then 3k savings in a ~75k file, I liked it in theory too, just wan't much of a benefit in practise.

@sheppard
Copy link
Contributor

sheppard commented Oct 2, 2013

It's not so much the physical size difference I have in mind, but rather the conceptual overhead that goes into reading the built file. When looking at an AMD bundle, I can easily see the module definitions and trace them back to their original files. When I look at a browserify bundle, the first thing I see is a bunch of minified boilerplate, and then a bunch of modules indexed numerically. Since most people are going to be interacting with the built file and not the source, I think the aesthetics of the non-minified build are important. (Yes, this argument breaks down a bit when you bundle almond with the build. Can't we just get everyone to use AMD loaders ;) ? )

30 files actually isn't that much to load via AMD - ESRI JavaScript API projects (which use Dojo AMD under the hood) do it all the time. That's partly because ESRI hasn't make it easy to do local builds (yet), and I would obviously use the built proj4 in a production environment. However, when investigating browser issues in individual modules on a local environment, it's pretty valuable to be able to load them from source directly. (Source maps are also a way to achieve this, though only in modern browsers.)

At any rate, I can still use the new build in my AMD environment, so in some sense it shouldn't make a huge difference what proj4js is using for internal modules. I'm more concerned with the ripple effect - the more people get used to using browserify magic to hack browser modules together, the less likely AMD will ever be adopted widely enough to actually be useful. New developers are probably most likely to choose the same build systems as the libraries they use, even if other options would better fit their use case.

Suppose I should finish that blog post I was going to write...

@calvinmetcalf
Copy link
Member Author

I don't think AMD is dead as much as it's being pushed up a level with app developers (me included) using it even more but the nitty gritty parts of it being abstracted away from library developers with tools like browserify plus component which is actually amd behind the scenes but requires no defines on your part and curl which will load commonjs modules wrap them in defines and execute them for you.

@sheppard
Copy link
Contributor

sheppard commented Oct 2, 2013

Fair enough, I'll try to spend some time with these other tools so I can get more used to them.

Calvin Metcalf and others added 6 commits October 4, 2013 11:49
Because when inside vendor/lib or /views/lib, erica is unable to push for
instance constants.js AND constants/ since they become identical prop names.
@calvinmetcalf
Copy link
Member Author

@sheppard just for you I changed the amd.html test so that it it loads each of the files individually despite still being commonjs

@calvinmetcalf
Copy link
Member Author

@ahocevar @sheppard ping

@ahocevar
Copy link
Member

I have no strong opinion on this, because I simply don't know enough about it. If @sheppard finds it useful, then I don't see a reason why this change shouldn't be merged.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

Sorry, even after npm clear cache and deleting node_modules, I cannot make grunt-browserify happy.

@calvinmetcalf
Copy link
Member Author

Try upgrading node

npm install -g n

n stable

Then install browserify locally

npm install browserify

Then install all

npm install
On Nov 5, 2013 10:30 AM, "ahocevar" notifications@github.com wrote:

Sorry, even after npm clear cache and deleting node_modules, I cannot
make grunt-browserify happy.


Reply to this email directly or view it on GitHubhttps://github.com//pull/67#issuecomment-27781881
.

@sheppard
Copy link
Contributor

sheppard commented Nov 5, 2013

Sorry for the delay, I've been caught up in grad school stuff and my other projects. I guess I am still not ready to support the change to browserify/CommonJS in general, for example this gist by @jrburke covers some of my concerns on the issue. (There are some pro-browserify counterpoints in the ensuing discussion as well). If proj4js was only intended for browser use, I might continue to push for sticking with AMD. However, since there is a strong node use case as well, I recognize that prioritizing node's module system may make more sense for this particular project.

More to the point, I don't want to hold things up, so I'll defer to @calvinmetcalf on this one.

@sheppard
Copy link
Contributor

sheppard commented Nov 5, 2013

I admit curl's CJS loader is pretty nifty, though (like any AJAX-powered loader) it isn't as robust as the <script src=> tag injection used for AMD modules.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

@calvinmetcalf even with your instructions and npm v0.11.8, I'm not able to satisfy this dependency.

@calvinmetcalf
Copy link
Member Author

@ahocevar you should be at node version 0.10.21 not 0.11.8

@sheppard yeah it isn't the best option, I do think it is on balance the least worst one for synchronously building something

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

I think dist/proj4.min.js can be deleted now.

@sheppard
Copy link
Contributor

sheppard commented Nov 5, 2013

Re file names - note that Leaflet doesn't check in dist JS anymore.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

@calvinmetcalf Same with 0.10.21, sorry.

@calvinmetcalf
Copy link
Member Author

@ahocevar you've got the latest commit?

@sheppard I've only been checking them in when I publish a tag, I think i should start doing that in a build branch from now on.

@sheppard
Copy link
Contributor

sheppard commented Nov 5, 2013

I was able to build from a fresh clone, though I'm not sure what npm libs I already had installed globally.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

@calvinmetcalf Yes I have it, and I get browserify 2.35 installed.

@calvinmetcalf
Copy link
Member Author

@ahocevar then what error do you get ?

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

@calvinmetcalf Still the same, when running npm install:

npm ERR! peerinvalid The package browserify does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer grunt-browserify@1.2.11 wants browserify@>=2.35 < 3.0.0

@calvinmetcalf
Copy link
Member Author

npm uninstall -g browserify && rm -r node_modules && git pull origin browserify && n stable && npm install

if that doesn't work I am stumped

also is there another line in the error where it says what browserify is at?

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

Wait a sec. What is the source of this pull request? A few commits are missing in https://github.com/calvinmetcalf/proj4js/tree/browserify.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

Ah, we're pulling from upstream here. Let me try again.

@calvinmetcalf
Copy link
Member Author

ah things make more sense now, my old computer had calvinmetcalf/proj4js as origin and proj4js/proj4js as upstream, I'd occasionally push wrong thats where that comes from

@sheppard
Copy link
Contributor

sheppard commented Nov 5, 2013

Yeah this is an intra-repo pull request.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

Ok, finally! Now running tests locally.

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

Ok, tests passing, and lib works in my projects. Now one final question: if dist is in gitignore, how do we publish to npmjs.org et al?

@calvinmetcalf
Copy link
Member Author

my plan

git checkout -b build
git add dist/proj4.js -f
git add dist/proj4-src.js -f
git commit -m build
git tag $version
git push origin $version
npm publish
jam publish
git checkout master
git branch -d build

@ahocevar
Copy link
Member

ahocevar commented Nov 5, 2013

Sounds good. Can you summarize this in a "Release procedure" doc in the wiki or repository please? But the pull request is good to merge. Thanks for the hard work.

@calvinmetcalf
Copy link
Member Author

you should be able to do ./publish.sh 2.0.0 if it works I'll write it up

calvinmetcalf added a commit that referenced this pull request Nov 5, 2013
@calvinmetcalf calvinmetcalf merged commit 94dcf56 into master Nov 5, 2013
@calvinmetcalf calvinmetcalf deleted the browserify branch November 5, 2013 18:47
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

4 participants