esnext / es6-module-transpiler

Tomorrow’s JavaScript module syntax today
http://esnext.github.io/es6-module-transpiler/
Other
1.21k stars 85 forks source link

Allow writing a container multiple times. #163

Closed eventualbuddha closed 10 years ago

eventualbuddha commented 10 years ago

@caridy @tbranyen

What do you think of this approach to allowing multiple write calls? It's effectively making it more explicit that the contained modules are not re-usable once the conversion has happened. We still only format/build the result once, but we now cache the result and re-use it on subsequent calls to #write.

Closes #155.

caridy commented 10 years ago

Well, I haven't seen a case where we want to write the modules from a container into two different output folders, so I can't speak for that, maybe @ericf knows more. In the other hand, I don't see any harm on doing what you are doing here... therefore: +1

eventualbuddha commented 10 years ago

@caridy more generally, how do you think the Container should be used now that you have a pretty firm grasp on the internals of this project? I originally assumed it as a kind of one-shot thing, but that ended up being kind of an accidental design as the formatters can (and do) modify the module ASTs in a non-idempotent way. This at least makes Container#write something that can be called repeatedly without issue, but I wonder about things like whether we should we do anything to make Container#convert work on a copy or otherwise preserve the original AST information, etc?

caridy commented 10 years ago

this is a tricky question, so I will elaborate ...

we have around 6 different pkgs that are currently compiling to dist/ for bundle and lib/ for cjs, initially we were trying to achieve this by reusing the container, ultimately, started drifting from that initial idea in favor of a surprising revelation: it seems that when you're compiling to different formats you will probably need a different set of modules, often a custom entry point to the module graph to accommodate the exports to the output format, etc.

bottom line, even if we have a idempotent container that transform on the fly, we will probably end up creating one container per output format :/

ultimately, I think there is another important bit that we can consider, the fact that we are transforming source code, which has a unique absolute path. this make me think that probably we should have a static module store that can be used to cache all modules and re-use them from different containers, and even multiple passes from the same process (saying watch process...). something along those lines... I think caching a module is easier, simpler, but I might be wrong :)

eventualbuddha commented 10 years ago

Thanks for your thoughts, @caridy. Something along the lines of a caching module store is probably a win for things like broccoli, etc. If you have any insights into how we ought to pursue that I'd love to hear them.

ericf commented 10 years ago

I think where you guys ended up in this conversation :smile:

This at least makes Container#write something that can be called repeatedly without issue,…

:+1:

…but I wonder about things like whether we should we do anything to make Container#convert work on a copy or otherwise preserve the original AST information, etc?

Yes, this is very important. In my work on the Broccoli plugin that wraps the transpiler, this issue kept coming up. I wanted to at least cache the process of reading in a module file and parsing it into an AST. That way in incremental builds I could hand create a Module instance object and assign my original cached AST to its ast prop. So I think working on a copy of the AST makes sense. Although another approach would be for Recast to expose its AST copy function (which it runs as part of its parse step so it can use the original AST to build the source map.)

Thanks for your thoughts, @caridy. Something along the lines of a caching module store is probably a win for things like broccoli, etc. If you have any insights into how we ought to pursue that I'd love to hear them.

This sounds like a great idea to me, and it would help the Broccoli plugin tremendously. This combined with first class support for accept ASTs as input would be great! Maybe this could be implemented as a resolver? That's how I did it for the Broccoli plugin.