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

Use ES6 modules instead of CommonJS #1206

Closed
paf31 opened this issue Jun 29, 2015 · 133 comments

Comments

@paf31
Copy link
Member

commented Jun 29, 2015

ES6 modules have static imports and exports, which seem like a better fit.

This really seems like it would have been a good thing for 0.7, but it's a bit late for it now. Let's discuss it anyway.

@paf31 paf31 added this to the Ideas milestone Jun 29, 2015

@paf31 paf31 added the question label Jun 29, 2015

@flq

This comment has been minimized.

Copy link

commented Jun 29, 2015

Considering that ES6 is the future, it does make a lot of sense going forward with this.

@garyb

This comment has been minimized.

Copy link
Member

commented Jun 29, 2015

The main tradeoff here is this further complicates the build process for people targeting the current web, as now you need to use babel or some other tool to produce ES5 code. Also does language-javacript support ES6 yet? If not we'll have some work to do for psc-bundle 😉

@paf31

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2015

you need to use babel or some other too to produce ES5 code

That's definitely a downside.

Also does language-javacript support ES6 yet?

I don't think so, but it would probably be easy to either patch it, or even just hack around it.

I think this can just be something to keep an eye on.

@paf31 paf31 changed the title Use ES6 modules instead of CommonJS? Use ES6 modules instead of CommonJS Mar 10, 2016

@paf31 paf31 removed the question label Mar 10, 2016

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

Any more recent thoughts on this? Perhaps we could use psc-bundle to compile ES6 modules down to ES3.

@texastoland

This comment has been minimized.

Copy link

commented Mar 10, 2016

I thought about this earlier today.

static imports and exports, which seem like a better fit.

My impression is that only matters for DCE which PS handles natively?

now you need to use babel or some other tool to produce ES5 code.

Yeah that's a tool's job. There are standalone ES6 module transpilers however.

Also does language-javacript support ES6 yet?

Nope ES5. I don't understand what it'd gain since they're interoperable with CommonJS anyway?

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

The big win is the fact that all module imports and exports are statically determined.

The modules we generate are already very much like ES6 modules, except for syntax. In fact, our DCE tool could serve as a general-purpose ES6 dead code tool if we could parse the syntax.

Also, ES6 modules are better suited for static analysis, so we would benefit from any new tools built going forward.

@garyb

This comment has been minimized.

Copy link
Member

commented Mar 10, 2016

Perhaps we could use psc-bundle to compile ES6 modules down to ES3.

I assume you're just talking about rewriting the module require syntax and not reimplementing Babel? 😜

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

Yeah, I'd like to do the minimum possible to make ES6 modules work. As for DCE, I would hope that we could actually start using some dead code tool for ES6 instead one day.

@natefaubion

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

There's rollup https://github.com/rollup/rollup and the upcoming webpack release will support DCE for es6 modules.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

@natefaubion Thanks, do you think if we targeted ES6 modules then it would be able to replace psc-bundle? If so, I think that's a good argument in favor of ES6 modules.

@natefaubion

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Rollup supports AMD/CommonJS/ES2015/Globals/UMD output, so I'd say its definitely more flexible. The downside is that you will have to do some sort of translation of the compiler output to get your code to run in node, which is not required right now. This is something that pulp could setup easily enough (if using pulp run), but it means output is not immediately runnable for those not using the recommended tooling :/

@texastoland

This comment has been minimized.

Copy link

commented Mar 10, 2016

I suppose you could generate CommonJS by default and ES6 with a flag (they're interoperable)? It does make sense to defer to Rollup for basic DCE.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

Well, it should be relatively simple to come up with a proof of concept using Rollup on the output of psc.

@natefaubion

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

I'd be happy to work up an ES6 POC (it's quite trivial if you just use the JSRaw constructor for the moment).

@natefaubion

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

One thing to note is that FFI files would need to be converted to ES6 modules to get DCE.

@texastoland

This comment has been minimized.

Copy link

commented Mar 10, 2016

🎉

@natefaubion

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

Turns out, rollup is really terrible at tree-shaking PS code. It's very conservative about what it will eliminate because JS isn't pure. So for example, all data constructors are wrapped in an IIFE, so it won't eliminate those because they may potentially have side effects. So it actually removes very little code, unfortunately 😞

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

Hmm, I was worried that might be that case. Oh well.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

I wonder how much real-world JS code out there is pure enough to benefit from such a tool.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

It's a shame because the code is obviously all there, it's just too conservative. I wonder how difficult it would be to fork the project and remove the checks. If that could be done and upstreamed with a command line flag to enable it, then I think it'd be worth it to switch to ES6. That's probably a big if though.

@texastoland

This comment has been minimized.

Copy link

commented Mar 11, 2016

I wonder how difficult it would be to fork the project and remove the checks.

Why don't we open a ticket first? Might Webpack 2 make any better tradeoffs (supposedly less ideal for libraries)?

@paf31

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

Yeah, that sounds like a good idea 😄

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

My repo I've linked several times demos PS code tree shaking without modifying data constructors

Ok, great! Sorry, I didn't realise it was actually working in your test repo. In that case I don't see any major problems with moving to ES6 modules; there's still the issue of having to install some tool for the tests but I think that's surmountable. If we can get Pulp to a stage where all the tests pass (it's pulp build --to that I'm particularly interested in) then I think this is worth doing now. It also might be worth thinking about how this affects purs-loader.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2016

@texastoland Are you referring to https://github.com/texastoland/purescript-tree-shaking-test? it would be useful to see some documentation, however small. I looked at rollup.js which seemed to be the most concise output, but it's not doing the same sort of DCE as psc-bundle. It's pulling in entire modules which are not needed. Am I missing something?

Also, there are users who do not use Pulp at all, so we should be mindful of those use cases as well.

@texastoland

This comment has been minimized.

Copy link

commented Jun 30, 2016

it would be useful to see some documentation, however small.

I plan to when I start generating code instead of manual modifications.

I looked at rollup.js which seemed to be the most concise output, but it's not doing the same sort of DCE as psc-bundle.

No general tool will ever replace psc-bundle as discussed at Hacked cf. #1206 (comment). Also we chose WebPack for integration with various tools not conciseness. Rollup doesn't aspire to do anything but tree shaking.

It's pulling in entire modules which are not needed.

I'm not aware what you're referring to?

Also, there are users who do not use Pulp at all, so we should be mindful of those use cases as well.

That's why I added a flag. I'm interested in modern JS (const, =>, class) i.e. #2207 inclusive of modules cf. #1206 (comment).

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@texastoland can you elaborate on why you think no general-purpose tool can replace psc-bundle? I was under the impression that we would stop using it after we implement es6 modules; in fact I though one of the main pros of es6 modules would be not having to maintain psc-bundle any more. It would be quite a lot of effort to make psc-bundle work with ES6 modules as someone would have to update language-javascript to understand at least ES6 imports and exports (although I guess the maintainers would rather have a library capable of parsing either ES5 or ES6, and probably not some odd language halfway in between the two).

Regarding whole-program optimisations such as the uncurrying one, I think we should try to write this kind of stuff in PureScript, since JS seems to be the language that all JS language tools are written in nowadays.

I'm not keen on generating classes or arrow functions in psc unless someone can provide a good reason to. The rationale for classes was supposed to be improved DCE because the IIFEs are considered side effectful, but apparently they're fine after all. The rationale for arrow functions was code size, but the answer there is compression (cc @michaelficarra).

@texastoland

This comment has been minimized.

Copy link

commented Jun 30, 2016

can you elaborate on why you think no general-purpose tool can replace psc-bundle?

Ah I never looked at the project before so I assumed it worked in phases like Scala. No tool could ever replace one that optimizes the AST.

I think we should try to write this kind of stuff in PureScript,

In that case parsing modern JS would be a solved problem.

unless someone can provide a good reason to.

How about the converse. Why not? Why not emit what's most readable and downgrade for compatibility? Why do other popular JS compilers do the same? Or why bother maintain readable JS? It's not FFI alone. Those weren't my motivations and didn't sound like @michaelficarra's.

@wegry

This comment has been minimized.

Copy link

commented Jun 30, 2016

@texastoland Rollup ostensibly provides the ability to go from CommonJS and AMD to ES6 modules. I can't imagine it would be good for build times though.

At least in your example repo, it would potentially remove those additional FFI files because as ES6 modules instead of CommonJS they could be tree-shaken.

@texastoland

This comment has been minimized.

Copy link

commented Jun 30, 2016

Ohhh that means versioned FFI files would be unusable without transpilation regardless what psc emitted…

@wegry

This comment has been minimized.

Copy link

commented Jun 30, 2016

Wouldn't it be the same for the psc transpiled PS though? Transpilation isn't necessary with rollup (which looks wonderful by the way) or webpack@^2 (edit) for modules. It's only converting the module and export syntax. Neither of which are supported by any current browser except for a dev build of Edge.

Edit 2: Nevermind, missed the arrow function mention.

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

Because the code produced by psc is executed much more often than it's read, and it's only a very minor readability improvement for another compile step, which is probably easy with some setups but might be really painful in others. It just doesn't seem like a worthwhile tradeoff to me.

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

No tool could ever replace one that optimizes the AST.

I'm still not quite sure what you're getting at here.

@texastoland

This comment has been minimized.

Copy link

commented Jun 30, 2016

I'm still not quite sure what you're getting at here.

E.g. inlining.

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

Ahh, gotcha. Is it feasible to have a program that just performs whole-program optimisations like uncurrying on a bunch of es6 modules, and then perform DCE on the result of that with a separate tool, like Rollup? We're tantalisingly close to being able to ditch psc-bundle.

@texastoland

This comment has been minimized.

Copy link

commented Jul 1, 2016

I imagine so but…

Ohhh that means versioned FFI files would be unusable without transpilation regardless what psc emitted…

I don't understand the objection to modern JS based on adoption (http://node.green and http://kangax.github.io/compat-table/es6/) nor about different emit modes but…

Standardizing on modules exclusively would require FFI to be postprocessed to be consumed. What if it isn't migrated from CJS? Must is still wrapped by Browserify or another bundler for browser anyway? What about PSCi which doesn't use Pulp?

Has anyone documented a standard for FFI runtime? I think it's informally ES5 and CJS but what about available libraries? Is the environment polyfilled or not? global or window? Working with Aff's FFI I disagree that's ES2015's readability is a minimal improvement.

I disagree that such a flag would break semantics. PS projects depend on PS source. It doesn't affect FFI or even instanceof checks. TypeScript has provided flags as long as they've been advancing standardization. Bundlers have been abstracting modularization even longer. IIUC PS has always been dependent on external tools to run in the browser. I think neither embracing standardization nor compatibility would hurt a project whose biggest selling point besides typed pure FP is integration with JS as a platform.

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

I mean, of course we are getting there with browser support for things like arrow functions, but we're definitely not there yet. You'd still need to compile to ES5 in many (possibly even most) cases.

I think if we were to start generating ES6 modules, then we would require FFI to use ES6 modules too, at least for exports. This would be a painful breaking change but we could probably hack together a tool to convert from the current commonjs style automatically.

You'd still need to use Browserify or some similar tool if you wanted to pull node modules into a browser bundle, as is currently the case.

Psci would indeed be difficult, that hadn't occurred to me.

I don't think it's possible or even desirable to come up with some kind of documentation about what's available in FFI modules. Bindings to browser APIs have to take things from the global context, and bindings to node APIs have to assume that the relevant node modules (eg stream, fs) are available. I think we just have to document any assumptions made in FFI modules manually.

If you were to use arrow functions in Aff's FFI, then you'd also have to put in its README that it uses arrow functions and therefore anyone using Aff might need to compile down to ES5. Someone's setup might break just by adding an Aff dependency, for what seems like an avoidable reason. I agree that ES6 would make the code more manageable, but for such an important library, I'd guess it would cause a lot more pain in the long run to use ES6. Perhaps you could consider writing the FFI in ES6 but then compiling to ES5 during npm run build and checking the result in? Once we move off bower we could maybe make this nicer by providing a way to include build products in source distributions without having to check them in to the repository.

I don't remember a time when PureScript was dependent on external tools to run in a browser, btw. Note that you only need browserify if you want to include node modules in your browser bundle. Otherwise psc-bundle is sufficient. Before we had psc-bundle the same functionality was inside psc itself.

The argument is not that adding flags for multiple output modes would break any semantics, just that they are a massive pain in practice. Good examples of this are psc-make as well as the --require-path option, which we've now removed and I'm very happy they're gone. If TypeScript is happy to take on the extra work of supporting multiple output modes on then all power to them, but I don't think it's a good option for us.

I think if we went down the ES6 module route and ditched psc-bundle, we could say using ES6 features in FFI modules is absolutely fine, as long as you're writing an application (I think we should recommend libraries avoid it for now just because of lack of support, although it's probably not worth trying to enforce this). It would then be your own responsibility to have your own build process set up to deal with it, or to make sure your JS implementation can cope with it. Assuming psc itself only produces ES5 (which I think it should for the time being), then ES6 language features are opt-in, and people who don't want to spend extra time configuring their build process to cope with them aren't forced to do so.

@texastoland

This comment has been minimized.

Copy link

commented Jul 1, 2016

I think if we were to start generating ES6 modules, then we would require FFI to use ES6 modules too, at least for exports.

I didn't mean to suggest it ought to be required since bundlers including Rollup via plugin understand CJS.

Psci would indeed be difficult, that hadn't occurred to me.

My flag fixes it.

I don't think it's possible or even desirable to come up with some kind of documentation about what's available in FFI modules.

That's what I was leading toward. Rather than constraining users to unspoken arbitrary constraints why not set the expectation at standards. If tools like Pulp care for backwards compatibility it could be provided with Rollup creator's Bublé. (Not entirely convinced myself but thinking aloud.)

If you were to use arrow functions in Aff's FFI, then you'd also have to put in its README that it uses arrow functions and therefore anyone using Aff might need to compile down to ES5.

Addressed by previous statement.

Note that you only need browserify if you want to include node modules in your browser bundle. Otherwise psc-bundle is sufficient.

That's what I meant by external tools.

they are a massive pain in practice.

Outside of modules and missing ADTs I suspect having already started last weekend we're talking about 20 LOC.

@hdgarrood

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

Maybe we don't have to require ES6 modules, but I feel like we'd be in a better position if we did. It's easier to cope with a uniform bunch of ES6 modules than it is to cope with a mixture of ES6 and commonjs modules.

Re allowing ES6 in FFI: not everyone uses pulp; that's where the issue arises. I think there's almost no assumption about purescript build processes that is safe to make. I know there are people who invoke psc directly, for instance. Yes, we could just tell everyone to expect that FFI modules contain ES6, modify Pulp to be able to cope, and tell everyone not using Pulp that they have to work something out to deal with it themselves, but I just don't think it's worth all the confusion it would cause.

When I said adding flags was a massive pain, I didn't mean to implement. It's dealing with it in build tools and supporting it that is the painful bit. Situations arise where people are trying to use two tools together, one of which assumes the flag is off, and the other requires it to be on. This has happened before with eg the --require-path option.

@texastoland

This comment has been minimized.

Copy link

commented Jul 1, 2016

It's dealing with it in build tools and supporting it that is the painful bit.

Mine was modeled after source-maps which already touches similar code paths.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2016

My flag fixes it.

Which flag? Is this on a branch somewhere?

@texastoland

This comment has been minimized.

Copy link

commented Jul 1, 2016

It's in my local branch I started last weekend and commented about that you replied to. Planning to use it to test tree shaking with generated code. I'm working on a few WebPack projects extensions ATM though.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2016

Sorry, I thought you were referring to a compiler flag. It's difficult to keep up with these threads.

@texastoland

This comment has been minimized.

Copy link

commented Jul 1, 2016

Sorry, I thought you were referring to a compiler flag. It's difficult to keep up with these threads.

Yes I implemented a compiler flag based on the existing source-maps and named it based on @michaelficarra's suggestion. @garyb suggested using Reader to clean it up but I haven't considered the code impact yet.

@paf31

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2016

Closing as a duplicate of #2207.

@paf31 paf31 closed this Sep 25, 2016

@paf31 paf31 added the duplicate label Sep 25, 2016

@wegry

This comment has been minimized.

Copy link

commented Sep 25, 2016

For anyone that manages to make it through all the discord down to here, and you would like tree-shaken ES modules in PS 0.9.x and meet the following conditions, post processing is probably the easiest:

  • Only using psc (no psc-bundle)
  • Using Webpack 2 that natively understands ES modules without transpilation
  • All exports must be valid ES identifiers which I believe is true in 0.9 but as @texastoland has pointed out elsewhere, this remains undocumented. We're still on 0.8 at work, so I have been unable to use and test this on a sizeable project.

I wrote a not fully fleshed out or terribly robust PS flavored CJS to ES6 loader for Webpack 2.1 that uses regexes. I admit an AST based approach would definitely be preferable.

That said, at least for my tiny cost of importing Prelude to add two numbers together brings the cost of importing the Prelude to add numbers together down from 50 KB to 15 KB, because Show instances don't seem to shaken out for fear of removing side effects. That's been covered elsewhere too I believe.

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