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

Stop spamming npm #5

Closed
sindresorhus opened this issue Sep 2, 2014 · 25 comments

Comments

Projects
None yet
5 participants
@sindresorhus
Copy link

commented Sep 2, 2014

Please stop spamming npm with useless wrapper modules: https://www.npmjs.org/~requirify-bot

Use the Browserify CDN or something: http://wzrd.in/

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2014

Thanks for bringing this up. There was a little discussion on this at HN: https://news.ycombinator.com/item?id=8248027

Great functionality, but aren't you a teensy weensy bit worried about polluting the npm namespace?! I appreciate that you're prefixing your 'requirify' generated packages with 'requirify-' prefix, but something about this solution seems like it's abusing the npm tooling. At the very least it's forcing people to download (potentially much) larger npm indexes.
Maybe an npm maintainer can comment.

I'm part of the ops team at npm, personally I love the idea of people finding creative uses for the registry. Within the next few months we hope to add better ways to manage collections of packages on npm: Browserify packages, other-front-end packages, grunt packages, etc.
This is a cool proof of concept, and I think would be a great candidate for some of these new features.

I believe above quote is from @bcoe, maybe he can chime in further.

So far nobody has actually explained a compelling reason not to publish these to npm. I'm leaving this open for further discussion.

@sindresorhus

This comment has been minimized.

Copy link
Author

commented Sep 2, 2014

So far nobody has actually explained a compelling reason not to publish these to npm.

There's enough man-made junk on npm to make discoverability hard. Adding bot created junk only makes that worse. Even with better discoverability at some point, it IMHO doesn't make sense to allow bots put whatever useless things on there.

But ignoring that, why go through all the trouble when you can do this programmatically at runtime?

@sindresorhus

This comment has been minimized.

Copy link
Author

commented Sep 2, 2014

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2014

The main reason for doing this is to embed both the functionality of

  • fetch code from a remote server
  • export that module to a variable in the console

in a function called require.

It isn't totally clear that it this can be done programmatically at runtime. I know you can append a script pointing to wzrd.in to the page and then use that scripts exported require() function, but then it is a two step process.

Maybe one possibility is to have a require function that stores a reference to itself like

require = function(varName, moduleName) {
   moduleName =  moduleName || varName
    _require = require

    // fetch wzrd.in code
    // ...

    window[varName] = require(moduleName) // this uses the require from wzrd.in code

    require = _require   
}
@maxogden

This comment has been minimized.

Copy link

commented Sep 2, 2014

@mathisonian I dont fully understand what's going on here but it sounds like you should use the /bundle api instead of the /standalone api on browserify-cdn. I don't understand why you need to publish new modules to npm yet (still wrapping my head around this). I agree with @sindresorhus that npm should be for human-published code though.

@jdalton

This comment has been minimized.

Copy link

commented Sep 2, 2014

_<[lodash-modularized](https://www.npmjs.org/browse/keyword/lodash-modularized)>_
@maxogden

This comment has been minimized.

Copy link

commented Sep 2, 2014

@jdalton I actually thought of lodash when I was writing my previous comment. IMO it's a different use case -- lodash-modularized lets people who consume lodash have more granular dependencies. I am not sure what the positive effect is of the requirify approach though -- it seems like it could be accomplished by other means than publishing new modules.

@jdalton

This comment has been minimized.

Copy link

commented Sep 2, 2014

@maxogden 😀

@jdalton

This comment has been minimized.

Copy link

commented Sep 2, 2014

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2014

Thanks @maxogden.

So to make it clear - I'm not married to this approach at all for requirify, I don't care much how it works as long as it works.

The reason that /standalone is used is to avoid clobbering requirify's require() method when a module is pulled down from wzrd.in. That is, the library should support multiple require calls, not just one.

screen shot 2014-09-02 at 7 31 19 pm

I haven't had a chance to confirm that the pseudo-code in my previous comment would actually solve this issue, but if it does then I'm fine changing it to that.

For reference for anyone stumbling up this: https://gist.github.com/mathisonian/c325dbe02ea4d6880c4e explains how everything works currently.

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2014

@jdalton

This comment has been minimized.

Copy link

commented Sep 2, 2014

Yikes!
I made this, npmjs.org/package/requirify-this-is-spam-this-is-spam, by simply goofing around with the extension doing require('this-is-spam') in my Chrome web console.

Super clever & creative, but not cool... too much power and potential for abuse.

@maxogden

This comment has been minimized.

Copy link

commented Sep 2, 2014

@mathisonian just so we're on the same page:

if you do this:

<script src="http://wzrd.in/bundle/concat-stream@latest"></script>
<script src="http://wzrd.in/bundle/split@latest"></script>

then the first one defines window.require and also adds concat-stream to the internal require scope. the second one uses the existing window.require (doesn't clobber) and adds split to the scope. so now you can type into that page either require('concat-stream') or require('split') and it will work

so could you accomplish your goals by just making a function that fetches /bundle output from wzrd.in and evals them? you wouldn't be able to use require but IMO that is silly anyway since your require has different semantics (e.g. it's async)

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2014

@jdalton Okay I'm just going to disable the thing for now while we figure out a reasonable approach

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 2, 2014

@maxogden yeah I'm on the same page with that, it just loses the magic that a require one liner provides (for me the sync semantics don't make a big difference in interactive console environment).

Maybe the most reasonable thing is to just change the method name to requirify instead of require.

@maxogden

This comment has been minimized.

Copy link

commented Sep 3, 2014

@mathisonian now that I think about it you might be able to define a require function that still works with the require that browserify /bundle output defines.

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2014

@jdalton fwiw npmjs.org/package/requirify-this-is-spam-this-is-spam isn't possible with the fix from #3 pushed to the server.

Still doesn't help with https://www.npmjs.org/package/requirify-fuck-lodash though

@maxogden I'm still not convinced

require = function(varName, moduleName) {
   moduleName =  moduleName || varName
    _require = require

    // fetch wzrd.in code
    // ...

    window[varName] = require(moduleName) // this uses the require from wzrd.in code

    require = _require   
}

wouldn't work. Or does browserserify looking for existing window require mess this up?

@jdalton

This comment has been minimized.

Copy link

commented Sep 3, 2014

Still doesn't help with https://www.npmjs.org/package/requirify-****-lodash

The whole auto generate package should simply not happen. It's too risky.

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2014

I've changed the script to:

var request = require('browser-request');

window.require = function(name, moduleName) {
    _require = require;

    if(!moduleName) {
        moduleName = name;
    }

    console.log('Fetching ' + moduleName + '... just one second');
    request('http://wzrd.in/bundle/' + moduleName + '@latest/', function(er, res, body) {

        require = null;
        eval(body);
        window[name] = require(moduleName);
        require = _require;
        console.log('Finished getting ' + moduleName);
    });
};

which seems to have the same functionality and no module spam.

@mathisonian mathisonian closed this Sep 3, 2014

@jdalton

This comment has been minimized.

Copy link

commented Sep 3, 2014

Rock! Can the created npm packages be removed, is there a reason to keep them around?

@KenanY

This comment has been minimized.

Copy link

commented Sep 3, 2014

They can only be unpublished. They'll still appear in search results (npm/npm#4848, npm/npm#5645.

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2014

Yeah, no reason to keep them at this point

@sindresorhus

This comment has been minimized.

Copy link
Author

commented Sep 15, 2014

@mathisonian so, can you unpublish them?

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 15, 2014

@sindresorhus yeah, I'm coming heading home from vacation today - will get to it once I'm back.

@mathisonian

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2014

They should all be unpublished now, thanks all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.