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

npm deployment or a commonjs build #299

Closed
gnoff opened this issue Oct 4, 2016 · 9 comments
Closed

npm deployment or a commonjs build #299

gnoff opened this issue Oct 4, 2016 · 9 comments

Comments

@gnoff
Copy link

gnoff commented Oct 4, 2016

I may have missed it but it doesn't seem like this library is published anywhere as a commonjs (or UMD) module, nor is it available via npm or similar package managers.

Reading through the source it appears this could be remedied with an additional webpack config that has a commonjs or umd libraryTarget configured.

If this is a feasible option? I'm happy to attempt a PR if it is welcome

@chrissrogers
Copy link
Member

We avoid publishing to package managers in order to discourage production use of old versions. It's technically possible to reference the package via git using a number of package managers, but we want to discourage that and thus won't officially publish anywhere. Hope that's a helpful explanation!

@vojtech-dobes
Copy link

I am curious - for example us package manager (npm) can help us see outdated packages and therefore be the primary highlight for us that something has newer version. Why do you think package managers lead to using older versions?

@chrissrogers
Copy link
Member

With CDN deployment the update is immediate, but with any package it would be only as soon as the developer updates their dependencies.

@davidjoy
Copy link

davidjoy commented Nov 1, 2016

I had the same sort of question, and ran across this thread. Our situation:

We've recently spent some time investing in our build process and started using webpack and npm, which means we went from naively concatenating and minifying libraries to actual module bundling. I've been trying (without success) to bundle recurly.js into the rest of our code via webpack. An npm version with a commonJS module would have been great, but I couldn't even get it to work with the stock recurly.js published on the CDN.

Every other library we use plays nice with this approach - recurly.js seems to make some assumptions about its context and that this will be a reference to global scope, which just becomes undefined in webpack-land (I've had no luck using imports-loader to get around this, for those familiar with the knitty-gritty here). Admittedly I haven't been able to track down exactly what's going on, but in general, attempting to import/require recurly.js in a webpack bundle causes a runtime error where global (defined near the top as being equal to this) is undefined.

Regarding the argument that not allowing package managers/modules discourages use of old versions: we have historically used recurly.js by downloading a copy and including it as part of our deployment artifact, along with all our other libraries. This helps us minimize requests, isolates us from third-party outages/SLAs, etc. I assume I don't really need to go into the advantages.

So anecdotally, despite any intention on Recurly's part to the contrary, we're still on v3. We were not aware of the argument stated in this Github issue, nor discouraged. I can't imagine we're alone. Furthermore, v3 seems to be functional if not supported (is it officially supported? It's still on the CDN.), as our site isn't broken.

Unless Recurly is going to dictate that the only valid version is the most current one - forcing all clients to scramble to upgrade whenever there's any sort of release - it's reasonable to expect that people are going to lag behind sometimes. We certainly are, not through any grand plan, but simply because we have lots of priorities for our business that require developers' attention.

If that's the case, no amount of discouragement-through-not-supporting-package-managers is going to change the fact that people are legitimately using older versions in production. And if we can concede that... gosh, an npm module would be really nice.

tl;dr, it seems like giving people more options for using the code is unrelated to Recurly's ability to notify customers of breaking changes/need to upgrade.

(Thanks for reading - and to be clear, I mean all this respectfully! Not trying to ruffle any feathers!)

@chrissrogers
Copy link
Member

Thank you for the thoughtful and well-argued note @davidjoy. I completely understand wanting to bundle recurly.js into your application dist. We chose early on to encourage all customers to always run the most recent version of the library at all times, and that a CDN distribution model fit that goal best.

v3 will receive necessary security updates, and its latest version will remain distributed over our CDN indefinitely.

We certainly expect, and observe in a minority of requests, old versions of recurly.js running in the wild. We make sure to account for this when updating the library's sub-dependencies (chiefly the tokenization and field processing libs that run in the hosted fields on v4).

A note that this being global is an artifact of the webpack build that we run to create the distribution version of recurly.js.

We won't support npm distributions for now, simply to help us achieve the highest possible distribution of the most recent version of the library.

You may wish to create a wrapper module that utilizes load-script to act as a conduit. Something like the following, which should work with webpack assumptions (I haven't tested this).

recurly-wrapper.js

import load from 'load-script';

let reference;
let stack = [];

export function wrapper (ctx) {
  if (reference) ctx(reference);
  else stack.push(ctx);
}

load('https://js.recurly.com/v4/recurly.js', err => {
  if (err) return console.error(err);
  reference = global.recurly;
  stack.forEach(ctx => ctx(reference));
});

app.js

import {wrapper as recurlyWrapper} from 'recurly-wrapper';

recurlyWrapper(recurly => {
  // fun recurly stuff
});

It might still impose the additional request, but I can assure that our region and often locally cached CDN distribution is very fast.

Again, thank you for taking the time to work with us on this.

@davidjoy
Copy link

davidjoy commented Nov 2, 2016

Thanks @chrissrogers!

Do you maintain backwards compatibility in updates for a given version? (i.e., within v3, or v4) I'm happy to suggest to the group here that we fall back to using the CDN version (the argument that it might contain security updates is compelling!), but obviously I could only do that if there's a reasonable guarantee that updates to v3 won't include breaking changes in the JS API itself.

(All that said, I'd also like to get us onto v4 ASAP, but that fruit is hanging a bit higher on the tree.)

@chrissrogers
Copy link
Member

We maintain backward compatibility within versions, yes. And I highly recommend using the CDN distributed version :)

Absolutely understood on v4. Soon we will have a new feature for that which should make styling the forms initially much much easier.

@gagalago
Copy link

Hi,
I just saw an older version of reculry.js in the wild. The guys, because no npm package was available, just download the script from the CDN one time and then bundled it inside the rest of their application code.
In this case, the npm package will be a better solution 😝 .
As we see in this example, a CDN is not the solution for user to be up-to-date because because when a user don't find what he is loking for. I think the best solution is to give to customer solutions that is in use in most platforms.If you allow more integration user didn't do crazy stuff because there is a solution for them.
For me, the big warning on the readme of this repository is enough.

@bhelx
Copy link
Contributor

bhelx commented Nov 16, 2016

I think most people start off right here: https://dev.recurly.com/docs/getting-started-1#section-include and I'd assume some people never look at the README. So I don't think having it in the README is enough. It might be a good idea to clarify right there that you MUST include the library in that way in the docs. Perhaps some kind of warning tag.

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

No branches or pull requests

6 participants