search

PencilCode / jquery-turtle

Turtle Graphics for JQuery
Other
44 stars 25 forks source link

Browserify implementation and preparation for splitting into several files. #104

Closed MarkusBordihn closed 7 years ago

MarkusBordihn commented 8 years ago

@davidbau This is the first step for #99 and I tried to leave most of the current structure untouched. The src/... folder is containing the real source code. The "jquery-turtle.js" and "jquery-turtle.min.js" are generated out of the src/...

Note: New CL to keep the git history in place.

MarkusBordihn commented 8 years ago

@davidbau Ok such as I tough I had to put the generated files in an separate dist/ folder, otherwise git will auto-matching the history to them instead of the file in the src/ folder.

You could check with git log --follow .\src\jquery-turtle.js that the history is there and that the generated files has no history.

MarkusBordihn commented 8 years ago

Found a better way for this without any additional variable.

For the records:

timaschew commented 8 years ago

here my feedback (last commit of you is not included) https://github.com/timaschew/jquery-turtle/commit/9358eb6e6bae868d6be063fb4cdfc20311cbf3b8

src/jquery-turtle.js - node js dist/jquery-turtle.js - jQuery standalone dist/jquery-turtle.min.js - jQuery standalone minimized

:+1:

MarkusBordihn commented 8 years ago

@timaschew

here my feedback (last commit of you is not included) timaschew@9358eb6

Sorry will be a little bit longer answer to explain the different things more in detail.

require('jQuery');

This is required to allow all possible use cases and will be translated to something like this after the bundling: (typeof window !== "undefined" ? window['jQuery'] : typeof global !== "undefined" ? global['jQuery'] : null);

Lets assume somebody wants to use the nodeJs version to compile one file which include jQuery. Without the require('jQuery'); statement nodeJs would not aware that jQuery is used / required and so there will be no optimization or other things done.

browserify-shim

Is needed for the above reason.

Removing Grunt as bundler

This is totally fine.

$.turtle() results into a string: "eval(see.init())"

That's expected, but I would prefer that this will changed in future updates.

test are not working

I assume that the additional hooks to $. are not working. Tests working fine in this pull request.

src/standalone.js

Its not really needed if we keep "browserify-shim".

Suggestion

We should do this step by step, so after this pull request is implemented we could work on the next steps to include music.js and after this we should expose jquery-turtle over module.exports.

Use cases

Lets summarized the general use cases:

With the current number of files all of the above use cases are covered. Only the module.exports needs to be added in an additional CL to fully supporting NodeJs.

We should also not assume that jQuery is loaded by the end user every time. For webpages this would be true, but for applications based projects most of the time all packages are bundled together into one big file for performance reasons.

@davidbau Please let me know if you have any questions left to this pull request?

timaschew commented 8 years ago

require('jQuery');

In general I agree, but in this case, turtle is a plugin, and IMO it makes more sense to use jQuery here as an argument, it's up to the user how he get jQuery.

browserify-shim

I'm still not sure if this can be handled with browserify only and the --exclude argument, I will investigate.

Required jquery-turtle over npm in NodeJs with jQuery Required jquery-turtle over npm in NodeJs without jQuery Bundled jquery-turtle over Browserify with jQuery Bundled jquery-turtle over Browserify without jQuery

I don't see a any cases for turtle + jquery in nodejs and for turte + jquery bundled as well.

Especially the first one doesn't make sense to me.

The second couples turtle to a specific jquery version. This is a bad idea, because you need always to update your bundle if there is a new jquery version and users want to use the new version + turtle. Leave out jquery give the user control when he want to use which jquery version.

MarkusBordihn commented 8 years ago

it's up to the user how he get jQuery

This is still the case even with browserify-shim, if the user is not include jQuery at all it will be defined as global variable, if he defines jQuery in his dependencies it will be bundled instead.

turtle + jquery in nodejs

Typical use case in nodejs projects, is someone wants to use the jquery-turtle library he will define something like this.

package.json

"dependencies": {
    "jquery-turtle": "https://github.com/MarkusBordihn/jquery-turtle/tarball/master",
    ...
}

If he decide to include jQuery as well he would simple add the line "jquery": "^2.2.0", so there is no need to have an additional <script src="../jquery.js"> or something similar in the nodejs app.

turte + jquery bundled

This is the typical use case for standalone, offline apps as well for sandboxes like webview.

I using exactly this use case in one of my projects: https://github.com/google/coding-with-chrome/

Which bundle jquery + cooffescript + jquery-turtle into one file for an offline application in the sandbox.

timaschew commented 8 years ago

If he decide to include jQuery as well he would simple add the line "jquery": "^2.2.0",

This is wrong by design in two parts. 1) When you provide an npm module, you never bundle it for the CommonJS usage with its dependencies 2) In this context jQuery is not just a normal dependency, it's a peer dependency. So the plugin should never provide its own version of the host (jquery).

so there is no need to have an additional <script src="../jquery.js"> or something similar in the nodejs app.

This is not related here.

This is the typical use case for standalone, offline apps as well for sandboxes like webview.

Yes, but the drawbacks which I described in the previous post are too big to let this case be valid.

This is still the case even with browserify-shim, if the user is not include jQuery at all it will be defined as global variable, if he defines jQuery in his dependencies it will be bundled instead.

Yes, but keeping a small module without any specific bundle related stuff sounds more clean to me. Because of the non CommonJS design of jQuery (instead pollute the global scope) I suggest not to require the peer dependency, instead pass it as an argument. This also makes tests easier.

MarkusBordihn commented 8 years ago

@timaschew Thanks for your feedback. This pull request is only for the Browserify implementation, if needed we should discuss the nodejs integration in a separate corresponding pull request, because its not really related to the changes here.

As I mention before, for me NPM and NodeJs are different things, that's the reason why I haven't include any module.export in this pull request.

If you want to add it later in an separate pull request I have no objection but I want to make sure that as many as possible use cases are covered.

I'm fine if an jQuery reference could be passed as optional parameter, but the user should not be forced to pass an additional parameter to jquery-turtle with the jQuery reference to keep it simple.

Please let me know if you agree and if we could relocate this discussion to an corresponding pull request instead ?

timaschew commented 8 years ago

:+1:

MarkusBordihn commented 8 years ago

Perfect ;)

@davidbau Feel free to merge this pull request if you have no other questions left ?

MarkusBordihn commented 8 years ago

@davidbau Friendly ping.

MarkusBordihn commented 8 years ago

Merged the latest changes. Would be great if this pull request could be merged backed in the next days, so that I don't need to keep an separate fork of jquery-turtle.

MarkusBordihn commented 8 years ago

Its seems that such implementation / split is no longer needed / wanted ?

MarkusBordihn commented 7 years ago

I will close this PR for now. Let me know if there is any interest for this solution in the future.