componentjs / component

frontend package manager and build tool for modular web applications
https://github.com/componentjs/guide
MIT License
4.55k stars 304 forks source link

breaking into smaller pieces #448

Closed ianstormtaylor closed 10 years ago

ianstormtaylor commented 10 years ago

we've all talked about getting lots of the logic in here broken into smaller pieces and refactor some of it. im super down to help on this stuff, but i wanted to make sure i was going about it in the right way before investing a bunch of time, so that's what this is. please offer thoughts if you have em

basically builder.js is a good step in the right direction, so i think we'd want to do similar things. for example we'd want a component/installer.js

but then there's a lot of logic right now (some of it repeated i think) that lives in component/component/lib/package.js that should probably be shared between installer and builder alike.

so it seems like we might end up with another one: component/package.js which would contain all of the package-specific stuff, like getting the json, getting the paths, saving the json, etc.

and then builder would just contain the extra necessary logic for building, thins like whether to build dev deps, grabbing local deps, whether to create symlinks. it already does a good job of this. and then installer would contain all the install specific logic, like output directory, versioning, etc.

does that make sense? or is there a better way? i'm happy to start making component/package.js and component/installer.js if so, or get help on it

dominicbarnes commented 10 years ago

+1 I've been wanting to do this for a while, but have also held back from investing time for the same reason. Is there anything I can do to help you out?

TooTallNate commented 10 years ago

+1

ianstormtaylor commented 10 years ago

k so i added two repos: installer.js and package.js. i'm no longer sold that package should be its own repo actually, maybe it should just be exposed from this one instead. happy to make that change if we decide that

@dominicbarnes or if anyone else wants to help, both of those could use lots more testing. and i probably made some dumb mistakes somewhere. among other things. now that installer is pulled out, i'd like to get it properly handling versions too and complaining when dupes with diff versions are installed

@visionmedia let me know what you think about all this. happy to keep working, or change what im doing if its all fucked up haha ;)

tj commented 10 years ago

+1 from me! no reason to couple the CLI and lib

lepture commented 10 years ago

This is a little off topic, but builder.js and installer.js have different code styles.

In builder.js:

function(){
}

but in installer.js

function () {
}
yields commented 10 years ago

+1 for function(){}

ianstormtaylor commented 10 years ago

+1 for function () {} hahahaha :)

i think i've come to the conclusion, after much OCD sadness, that that type of spacing is unfixable. there isn't enough of a default in either direction in the community, and differnt autocomplete tools do different things. it's not like if () vs if() which is much more slanted in one direction

(yes i think about this way too much)

dominicbarnes commented 10 years ago

+1 for function () {} hehe

yields commented 10 years ago

damn i think i'm loosing...

dominicbarnes commented 10 years ago

I get that it's only preference, there's not a right/wrong answer here.

Since the rest of component seems to use function(){}, then it seems appropriate to remain consistent.

lepture commented 10 years ago

@yields My fault! I shouldn't bring this up.

Actually, I prefer

function() {}

No white space after function, but white space before {.

But, my point here is that, since all other component projects use function(){}, I think installer.js should be the same.

jonathanong commented 10 years ago

style consistency is only valid within a project IMO, at least in open source. just try to respect the original author's style :).

jonathanong commented 10 years ago

i worked a little on a "resolver" that resolves all the locals and creates a list of component.jsons in correct order as well as resolves/consolidate dependencies based on spec changes. this would also be helpful for utilities like component-bundle and component-size since they all basically resolve everything themselves.

going to wait until @visionmedia releases the registry, though. right now, dependencies are resolved recursively with a LOT of HTTP requests. IMO, the registry should resolve all dependencies in a single HTTP request. not sure what direction @visionmedia wants to go with the registry, so i'm waiting :)

jonathanong commented 10 years ago

okay so i made three repos:

probably won't be done for a while, though.

ianstormtaylor commented 10 years ago

How does the flattening step work? Curious, instead of forcing the user to flatten before passing it into the builder, why not just let the builder accept the tree, and then have it flatten it itself if that is a necessary step for building.

One of my thoughts is that something called resolver complicates the system in general, because it seems like it takes on a lot of responsibility (hence why it doesn’t have a simple name to describe what it does) which seems like we could improve that somehow.

I like the current separation between install and build in the system. It makes it clear that for example we could just install when the server is first started, and then build on requests.

I might be thinking about this incorrectly though. What do you all think?

Random sketch of a way I think about it:

new Installer(__dirname)
  .remotes([local, github])
  .destination('components')
  .install(function(err){});

Those two remotes would be included by default (and would be their repository inside the component org so people can use them as well), but that’s the idea for how it can be extended. They would have a define API similar to how remotes does it. It could have useful statics for one-off installations:

Installer.install('path/to/dest', 'component/emitter', function(err){

});

Which is a “run-once” operation. And then the builder:

new Builder(__dirname)
  .scripts()
  .build(function(err, res){
    write('build.js', res.require + res.scripts);
  });

That is for just installing scripts. But if you don’t specify anything it will build with the entire spec:

new Builder(__dirname)
  .build(function(err, res){
    write('build.js', res.require + res.scripts);
  });

Seems like this is similar in a lot of ways to what you’ve built Jon, so just trying to figure out the differences and arrive at the best API for this stuff.

It feels like letting the builder doing the resolving, and letting the installer only worry about installing and downloading can reduce some of the complexity from resolver.

Let me know what you guys think.

jonathanong commented 10 years ago

this is a convo between @reinpk, @visionmedia, and @ianstormtaylor . feel free to chime in.

How does the flattening step work? Curious, instead of forcing the user to flatten before passing it into the builder, why not just let the builder accept the tree, and then have it flatten it itself if that is a necessary step for building.

flatten(tree) returns a list a flat list of nodes. the builder would then basically do nodes.map(plugin1).map(plugin2).join('') and return that result.

allowing just passing the tree to the builder is a good idea as most people don't need to manipulate the tree anyways. however, if this were the only way to do it, then building and splitting different bundles is a little more complicated. plus, it's nice to have component-flatten in a separate repo.

https://github.com/component/builder2.js/issues/28

One of my thoughts is that something called resolver complicates the system in general, because it seems like it takes on a lot of responsibility

well my opinion is that it "resolves the local components and remote dependencies", which includes installing them. installing is very easy (it's only a few more lines since most of that logic is in remotes and component-downloader) so i don't think it's overly complicated.

https://github.com/component/resolver.js/blob/master/lib/dependencies.js#L92

you can only resolve local dependencies with dependencies: false. i'm not sure how to split up local component resolution and remote component resolution into separate repos though. most of the logic in resolver.js isn't actual logic, anyways. it's populating objects and checking states for race conditions.

I like the current separation between install and build in the system. It makes it clear that for example we could just install when the server is first started, and then build on requests.

my philosophy now is that install is just the resolver. build is now just resolve + build. there's no difference between an install step's resolver and a build step's resolver. if everything is already downloaded, there's no "installations" happening during the resolve step (it'll only need to lookup the local remote), so it'll essentially be the same as the current build step.

the plus side is that if a component isn't installed when you build, it'll install it automatically. i personally like this, but it's easy to switch off. if i change a version and rebuild, the next build will work automatically.

right now. we're actually resolving the app in two modules, once during installation and the other during building. we only need one resolver.

jonathanong commented 10 years ago

there are too many ways to write the install/build flow. one change i just thought of recently is with remotes and locals.

Currently, the default is remote = remotes(['local', 'github']), where local and github are included. However, this is a little confusing when you have custom remotes and sometimes you don't want to check local, i.e. when you are checking if you have the latest version.

so i'm going to make local an option:

var remote = remotes({
  local: true
}, ['github'])

or something

jonathanong commented 10 years ago

It feels like letting the builder doing the resolving, and letting the installer only worry about installing and downloading can reduce some of the complexity from resolver.

oh no. it won't. the installing/downloading is all done in remotes/downloader and is only a few more lines in the resolver.

you're still going to going to need to build a dependency tree for both the builder and the installer. making it different in the builder and the installer will make it less DRY as well as more complicated if we want to support stuff like https://github.com/component/component/issues/483

jonathanong commented 10 years ago

we could also forget about the niceties of each module and instead make a component API:

var build = component
  .remotes(['github'])
  .set('root', process.cwd())
  .set('development', true)

build.scripts()
  .use()
  .write('build/build.js')

build.styles()
  .use()
  .write('build/build.css')
jonathanong commented 10 years ago

oh btw, for one-off installations, you can do:

resolve({
  dependencies: {
    "component/emitter": "*"
  }
}, { 
  install: true
})

kind of ugly, but it treats the first object like any other component.json. we can expose a cleaner API via component:

component
  .set('destination', 'components')
  .install('component/emitter', '*')
ianstormtaylor commented 10 years ago

for flatten i think we should figure out a way to have that not be something that needs to be exposed to the user, since that seems like an implementation detail to me. i'd prefer to remove the "bundling" use case from the initial builder implementation in terms of design. aka dont want the builder to have a weirder api if thats required just to support bundling, but i feel like it should be possible with small pieces to be able to easily support bundling without the api changing. as long as inputs and outputs are exposed everywhere it would just be piping basically

well my opinion is that it "resolves the local components and remote dependencies", which includes installing them. installing is very easy (it's only a few more lines since most of that logic is in remotes and component-downloader) so i don't think it's overly complicated.

the installing/downloading is all done in remotes/downloader and is only a few more lines in the resolver.

"resolves" doesn't actually mean anything in this context though, since it also means "downloads" and "semvers" and a bunch of other stuff. just because the logic is in another repo doesn't mean that it isn't being performed by the resolver. right now it seems to me like the resolver is trying to do too much, and having a build step that "might" install dependencies and might not ends up in the core api having weird side effects in different use cases

for remotes it seems like local wouldn't need to be an "option" if the installer just had a way to define remotes, and then throw the defaults in separate libs that are required in by the installer:

var local = require('component-local-remote');
var github = require('component-github-remote');
var Installer = require('component-installer');

var installer = new Installer(__dirname);

// the default
installer.remotes([local, github]);

// something custom
installer.remotes([myCustomRemote, github]);

instead of getting fancy with options:

var remote = remotes({
  local: true
}, ['github'])

oh btw, for one-off installations, you can do:

what about setting a directory for the destination to install? the problem i see with that syntax is that it's really verbose, i'd rather figure out an abstraction that keeps things simpler in general, and involve less nested objects or big options objects to work with

jonathanong commented 10 years ago

for flatten i think we should figure out a way to have that not be something that needs to be exposed to the user

yeah, i made flattening optional now. the builder will automatically flatten.

right now it seems to me like the resolver is trying to do too much

although i understand that concern philosophically, i don't see a pragmatic issue or a better solution. other than a nice API, what's the issue when you can just set dependencies: false, and not have the resolver resolve any dependencies? or install: false and not have the resolver install anything? i don't see any possible edge cases. having users use 5 different modules to do a simple task is not user-friendly imo, then we end up in substack territory.

an alternative is to have something like resolve-local-dependencies and resolve-remote-dependencies, but i tried that and it made things more complicated because you need each module to have it's own API, then glue all the APIs together. and it's harder for them to work together and be consistent.

i'd rather figure out an abstraction that keeps things simpler in general, and involve less nested objects or big options objects to work with

i'd rather not have abstractions in the core libs and keep the underlying code as simple as possible. we can always create a sugar API on top, which i think component should be, where we can argue about ideal API (which I agree, the current API isn't ideal). i'm just trying to get this out ASAP.

jonathanong commented 10 years ago

for remotes, the only issue is that some libraries want local as a remote, but some don't. and they can't just remotes.use(locals) because it has to be first. we could do remotes.prepend(locals) or something, but i don't really like that since libraries will basically be messing with people's remotes.

so for now i'm thinking that people create their own remotes through a function where all settings like locals can be intercepted:

module.exports = function myCustomRemotes(options) {
  var remotes = new Remotes(options);
  remotes.use(githubenterprise({auth, pass});
  remotes.use(github({auth, pass});
  remotes.use(bitbucket({auth, pass});
  return remotes;
}

then for a "outdated" command or something, you would pass the remote in like:

outdated(myCustomRemote, function (err) {})

where outdated does:

function outdated(Remotes, done) {
  var remotes = Remotes({
    local: true
  });
  // ...
  var remote = remotes.resolve('comopnent/emitter');
  var versions = yield remote.versions('component/emitter');
  // do stuff
}
jonathanong commented 10 years ago

closing for now. i have https://github.com/component/build.js for a nicer build api. open to more suggestions!