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

Allow usage with AMD/CommonJS #20

Closed
julkue opened this issue May 4, 2016 · 11 comments
Closed

Allow usage with AMD/CommonJS #20

julkue opened this issue May 4, 2016 · 11 comments

Comments

@julkue
Copy link

julkue commented May 4, 2016

Making the following necessary:

<script async defer id="github-bjs" src="https://buttons.github.io/buttons.js"></script>

means to also disallow usage with e.g. RequireJS. RequireJS will not create the id, but the same script tag. And this will cause nothing to happen.

@ntkme
Copy link
Contributor

ntkme commented May 4, 2016

Support for RequireJS means two things:

  • Hard code the buttons URL in config. This will create trouble for forks and pull requests.
  • Implement DOM ready event and detect if DOM is ready before the script is loaded to be fully async. This is an overkill. For simplicity, current async implementation requires the <script> to be placed after all <a> for buttons, in order to ensure the execution order.

@julkue
Copy link
Author

julkue commented May 5, 2016

@ntkme I can not say anything about what this means internally and I don't want to set you under pressure to allow usage with RequireJS. However, if you want to win users like me you definitely need to allow a usage like any other plugin (download with Bower and use with RequireJS), as making an embed to a remote .js file seems to be unnecessary too.

Regarding your second point, I don't know why this is an overkill for you

document.addEventListener("DOMContentLoaded", function(event) { 
  // initialize plugin
});

@julkue julkue mentioned this issue May 5, 2016
@ntkme
Copy link
Contributor

ntkme commented May 5, 2016

It's an overkill because no one knows the when the asynchronous JavaScript would be load. It can be before DOM ready, where you have to listen for the event (the method you mentioned requires IE9+). It can also be after the DOM ready, where listen for the event has no effect, you just execute the code.

The problem is that you want the code to be executed exactly once. Thus it requires a reliable way to detect if DOM is already loaded and listen for DOM ready.

@ntkme
Copy link
Contributor

ntkme commented May 5, 2016

Well, the DOM ready has been done anyways.

Why it's overkill? The compressed size of buttons.js has just increased 10%.

@ntkme
Copy link
Contributor

ntkme commented May 8, 2016

@julmot buttons.js should work with RequireJS now.

@ntkme ntkme closed this as completed May 8, 2016
@julkue
Copy link
Author

julkue commented May 9, 2016

@ntkme Thank you for your effort. Unfortunately I still can't use it. In most RequireJS workflows almond will be used in production. As almond does not support network resources, it would be necessary to download this package using e.g. Bower. So you would also need to make it available to download with Bower.

@ntkme
Copy link
Contributor

ntkme commented May 10, 2016

@julmot The support for Bower or any package manager is not going to happen.

This project is intended to be a reliable button service, but not a package. Packing it with Bower will simply prevent users from receiving updates and fixes automatically.


Although it is clearly against the design pattern of AMD, you can always write some JavaScript to inject <script> like following, and bundle that as a module:

(function(){
  var js = document.createElement("script");
  js.src = (/^http:/.test(document.location) ? "http" : "https") + "://buttons.github.io/buttons.js";
  document.getElementsByTagName("head")[0].appendChild(js);
})();

Alternative, if you really want to use a package manger, it is possible to install it with npm. You will need to install octicons in the right directory manually. You won't get any updates this way, and you must change the hard-coded CONFIG_URL in the minified script.

npm install https://github.com/ntkme/github-buttons/archive/master.tar.gz

@julkue
Copy link
Author

julkue commented May 10, 2016

@ntkme

This project is intended to be a reliable button service, but not a package. Packing it with Bower will simply prevent users from receiving updates and fixes automatically.

Well, this is up to the developer. You can choose "latest" in the configuration and will always get the latest version on every bower update. It is the principle and a convention to only update a library if you encountered problems. If you have any, then you can update the library. If you automatically receive updates you may not need then there is always the danger that something breaks in your environment, because you couldn't test it locally before deploying.

Also imagine the simple situation that at a time you require an attribute change. Because I would receive updates automatically the service would stop working for me. If you have realized it as a package then I could still use that explicite old version and are good to go.

@ntkme
Copy link
Contributor

ntkme commented May 10, 2016

@julmot I can totally understand your point of staying at a stable version with a package manager.

In fact, I tried Bower. It did not work.

Jaakko Salonen has written enough in Why We Should Stop Using Bower, but I have to repeat how terrible Bower is.

Since this project is hosted on GitHub Pages, all the front-end dependencies need to be committed to the repository or included as a git submodule. Using Bower or any other package manager means not no submodules, so I have commit the bower_components or node_modules into the repository, where it lose the point of using a package manager.

Ignoring the GitHub Pages for a moment, what if it is just a package for Bower? Well, Bower doesn’t support nested dependencies. This project requires octicons to be part of the building process, so it has to be installed with in the project. However, Bower insists on installing octicons as a sibling to this project, thus the URL to octicons stylesheet will be inconsistent therefore broken.

What about other package managers like npm?

As I said in a previous comment, a URL is hard-coded in order to support RequireJS per your request. This simply blocks package distribution from happening.

But why exactly?

buttons.js needs to know where the buttons.html for the iframe, and buttons.html will load buttons.js again (from cache) to draw the buttons in iframe. It is designed in this way to save network traffic.

In the previous implementation it uses the id attribute to get the src of buttons.js, thus it knows the relative url of buttons.html. Now since the URL is hard-coded, the iframe will always load //buttons.github.io/buttons.js in the iframe, which is not under your version control and might be different from the buttons.js you download in the package. It's redundant and things could just break easily.

This is a service.

Twitter buttons, Google +1 buttons, etc. Those are all provided as services. They never change their interface except for the visual looking part. This is what I am intended to do.

Changes will be backward compatible if they are going to happen. When any breaking change is really necessary, it will be something like buttons-v2.js in parallel with existing version.

@julkue
Copy link
Author

julkue commented May 10, 2016

@ntkme

where it lose the point of using a package manager

Using a package manager will definitely not automatically loose the advantages when checking the dependencies into a Git repository. There can be good reasons why you would check-in dependencies, even with a package manager. See for example this article.

Bower doesn’t support nested dependencies

I may not understand you here, but in fact Bower was intended to handle nested dependencies. If you download a jQuery plugin for example, bower detects that this plugin needs jQuery (because it was listed as dependency in bower.json) and downloads the plugin and jQuery it into a children directory of your choice.

buttons.js needs to know where the buttons.html for the iframe, and buttons.html will load buttons.js again (from cache) to draw the buttons in iframe. It is designed in this way to save network traffic.

Why not just have the default value pointing to the remote buttons.html on GitHub pages, but also allow setting a custom option for a local version of that file? Btw: This would need to be adjusted to also support the file-protocol (file:///):

"#{if /^http:/.test document.location then "http" else "https"}://buttons.github.io/"

@ntkme
Copy link
Contributor

ntkme commented May 10, 2016

Using a package manager will definitely not automatically loose the advantages when checking the dependencies into a Git repository. There can be good reasons why you would check-in dependencies, even with a package manager. See for example this article.

Git submodule can handle this case better.

I may not understand you here, but in fact Bower was intended to handle nested dependencies. If you download a jQuery plugin for example, bower detects that this plugin needs jQuery (because it was listed as dependency in bower.json) and downloads the plugin and jQuery it into a children directory of your choice.

Bower install all nested dependencies as sibling dependencies. No workaround to it at all. Although it does not make too much sense to keep dependencies nested like node_modules in the front-end, it is required for this project. octicons in this project is both a building dependency and a front-end dependency, where Bower only handles the later case.

Why not just have the default value pointing to the remote buttons.html on GitHub pages, but also allow setting a custom option for a local version of that file?

It has to work out of the box. Otherwise, users will open GitHub issues without reading the documentation.

The default remote URL can be broken as I have said in the previous comment. Giving an option in front-end means that you must set a global variable before this script is loaded, which is an extra dependency. Again, some people don't like documentation.

Btw: This would need to be adjusted to also support the file-protocol (file:///)

The protocol detection is only to avoid a mixed HTTP/HTTPS content security warning on very old IE. If the parent page uses file:///, that conditional statement will branch to https. All browsers have no problem loading resource via HTTPS on a page using File protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants