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

Support ES6 and beyond? #2207

Closed
michaelficarra opened this issue Jun 23, 2016 · 91 comments
Closed

Support ES6 and beyond? #2207

michaelficarra opened this issue Jun 23, 2016 · 91 comments

Comments

@michaelficarra
Copy link
Contributor

michaelficarra commented Jun 23, 2016

Inspired by #2204 (which I strongly support), I think we can avoid waiting for language-javascript to support modern JavaScript features by instead implementing a simple lexer. I don't believe we actually require the AST that language-javascript gives us. If we just look for the token sequences that indicate an export, we can infer that the name is exported regardless of context (thus not requiring a full parse). And if I'm not mistaken, the only reason we parse the FFI is to confirm that the required names are exported.

I'd be willing to do this work. Thoughts? Concerns?

edit: I forgot to mention, but this also allows adoption of future ECMAScript features in FFI without explicit support in PureScript, language-javascript, or some dependency. For instance, TypeScript/Flow annotations would pass through just fine.

@hdgarrood
Copy link
Contributor

I think we do need to know where any given export assignment statement ends for DCE in psc-bundle, which presumably would require some amount of actual parsing, rather than just lexing. It's perhaps still arguable that we don't need a full-blown parser to do this, but I'd personally rather depend on a more widely-used and separately released package than a not-quite-parser that only we use.

@michaelficarra
Copy link
Contributor Author

@hdgarrood I was thinking that DCE (and possibly all of psc-bundle) might be unnecessary after #2204 and related. We already have tools that do this for sufficiently declarative JS.

@hdgarrood
Copy link
Contributor

Good point. Did you perhaps mean to link to #2206 though?

@hdgarrood
Copy link
Contributor

Also, I am 90% sure that you are indeed correct - that in psc, we only parse FFI to ensure that the required names match up with what's in the PureScript module. How complicated do you think a JS lexer would be?

@michaelficarra
Copy link
Contributor Author

@hdgarrood Yes, "and related" was meant to include #2206 and any similar issues that are spawned from #2204.

Also, I am 90% sure that you are indeed correct

That's about how sure I am as well. Confirmation from @garyb would be helpful here.

How complicated do you think a JS lexer would be?

Well I have a significant amount of experience working on JS parsers, so I think I have a pretty good idea how difficult a standalone JS lexer will be, but I'm not sure how I can convey that to you. Are you looking for number of lines of code or something?

@garyb
Copy link
Member

garyb commented Jun 23, 2016

I'm also only 90% sure! @hdgarrood wasn't it you who added the FFI parsing to check exports? I didn't think we did any FFI parsing before that, aside from the ad-hoc find // module comment thing.

language-javascript was initially added only for psc-bundle if I remember rightly.

@hdgarrood
Copy link
Contributor

It was me who added it, yes, and it was the only part of psc that parsed FFI modules when I added it, but I'm not 100% certain that we haven't added more code since then that does other stuff with it.

@michaelficarra I was aware of your experience :) I guess I'm asking how easily someone else might be able to read through the code and fix a bug, were we to find one.

If we go for ES6 modules and try to reuse existing JS tooling as much as possible, which I think is very probably going to happen at some point anyway, we might also consider dropping this feature from psc, and making build tools responsible for integrating something to do this check if people still want it.

@hdgarrood
Copy link
Contributor

For instance, if you were running a DCE tool, that would probably catch this anyway, right?

@hdgarrood
Copy link
Contributor

("this" being an error where you foreign import something that isn't exported by the relevant module.)

@michaelficarra
Copy link
Contributor Author

@hdgarrood That is a very good point. It does seem like an awful lot of effort to go through to do something that an external tool is probably going to be doing anyway.

@paf31 paf31 added this to the Discussion milestone Jun 24, 2016
@paf31 paf31 added the codegen label Jun 24, 2016
@michaelficarra
Copy link
Contributor Author

The recent PR #2210 gives a reason to continue finding FFI definitions. psc-ide-server can return the definition location of a value. This is helpful for IDE jump-to-definition functionality.

@texastoland
Copy link

texastoland commented Jun 27, 2016

With a new lexer that just identifies exports? It's a bummer we can't use Babel or TS for FFI because of language-javascript.

@michaelficarra
Copy link
Contributor Author

With a custom lexer that just identifies exports though?

Yes. What's wrong with that? That's what I've proposed in this issue.

@texastoland
Copy link

Just confirming 💓

@paf31 paf31 changed the title support ES6 and beyond features in FFI while also dropping language-javascript dependency Support ES6 and beyond features in FFI while also dropping language-javascript dependency Jul 11, 2016
@paf31 paf31 mentioned this issue Sep 24, 2016
@paf31 paf31 changed the title Support ES6 and beyond features in FFI while also dropping language-javascript dependency Support ES6 and beyond Sep 24, 2016
@paf31 paf31 changed the title Support ES6 and beyond Support ES6 and beyond? Sep 24, 2016
@paf31 paf31 added the question label Sep 24, 2016
@dimsmol
Copy link

dimsmol commented Sep 26, 2016

Getting back to the matters brought up by #2204 and #1206 (this issue is linked as a continuation of them), I would say that generating modern JS and relying on the tools from JS universe (webpack, babel, etc.) may look like a good idea, but in practice it isn't that good.

Both webpack and babel aren't really reliable, neither they are fast. We have a huge project that takes more than 4 minutes to build. We are systematically suffering from all kinds of the nasty bugs coming from both webpack and babel or one of their dependencies.

Every npm-shrinkwrap.json generation for a dependency update is a risk of destabilizing everything because a tiny bug in any of the myriads of dependencies and sub-dependencies used by webpack/babel can break something. With npm, there is no good way to prevent these dependencies from switching to higher (and probably broken) versions even if you are updating just some unrelated stuff in your package.json

Maybe webpack 2 is more stable, I don't know. But I doubt it is because it's really hard to write reliable software in JS and there is no good quality control for all those dependencies. Both webpack and babel have a lot of code full of that JS-style interlinked data bags being passed around, especially where it relates to plugins and loaders.

Modern JS is something that cannot work without further processing, and unfortunately there is no really robust tools for this. With all my respect to webpack and babel I'd try to avoid them if I could. However, my current project is JS mostly and I cannot do it anyway.

Direct ES5 generation is faster and more robust than ES6 generation with further processing by babel or any other transpiler. And it's much easier than writing our own robust ES6 to ES5 transpiler tool.

As for bundling, @wegry shared his experience in #2204. We have very similar case (huge JS code base with growing but still relatively small PureScript part). But we have very different results. In our case, using purs-loader with PureScript bundling and DCE reduces resulting code size very well comparing to using purs-loader without bundling.

The difference is that we avoid depending on JS code from PureScript as much as we can. This allows PureScript bundler to optimize PureScript part and webpack to optimize JS modules independently. As it was already said, JS tree shaking will hardly be as effective as PureScript-aware DCE (or will require "tree shaking-optimized" code generation which can be suboptimal in many other ways).

Modern JS is good, but it's also good to be able to avoid depending on webpack/babel. From the practical standpoint, it will be very nice to preserve support for ES5 + CommonJS code generation and PureScript-aware bundling we currently have.

@Pauan
Copy link

Pauan commented Sep 29, 2016

I agree with @dimsmol

I also do JavaScript development with Webpack/Babel/Rollup. They work well enough for JS code, but I have had issues with breakage.

On a fundamental level, a PureScript bundler will be able to perform optimizations that a JS bundler will never be able to do. This is because of PureScript's static + pure behavior, compared to JavaScript's dynamic + impure behavior. In addition, a PureScript bundler has a much higher level understanding of the code (e.g. function type specialization, inlining, currying, etc.)

PureScript will need to support ECMAScript 6... just not right now. When ES6 becomes more mainstream (enabled by default in Node.js and/or modern browsers), then we will need to migrate over. But for now, I think the status quo is okay. You can use purs-loader and Babel if you want full transpilation.

On the other hand, I think it would be good to grab some low-hanging fruit: it would be nice if PureScript would parse ES6 in FFI files, so that it would work with tools like psc-bundle and psc-ide-server.

In other words, rather than doing full ES6->ES5 transpilation, it would simply parse the import and export statements. This is similar to what Rollup does.

@Pauan
Copy link

Pauan commented Sep 29, 2016

I have the following concrete proposal:

  1. Add in support to psc-bundle for bundling ES6 files. A file is detected as ES6 if it contains either import or export, otherwise it's assumed that it is CommonJS.

    It would not do any transpilation, so only import/export would be supported. This is just like how Rollup works. If you want transpilation you would need to use Babel, just like right now.

    psc would continue to output CommonJS and psc-bundle would continue to bundle CommonJS files. So this is a backwards-compatible change which adds ES6 module support, it does not replace CommonJS.

  2. After Node.js and/or browsers support ES6 modules, then change psc to output ES6 modules.

    Because of step 1, psc-bundle already supports ES6 bundling, so it doesn't need to change.

    Existing FFI code doesn't need to change either, because psc-bundle supports both CommonJS and ES6 modules. So this should be a backwards-compatible change.

    At this point we can also consider having psc output other ES6 features (e.g. class, =>, etc.)

  3. Slowly phase out CommonJS support from psc-bundle. PureScript FFI code would need to migrate over to ES6 modules (if they haven't already).

Step 1 shouldn't take that long, it just requires somebody to do the work.

Steps 2 and 3 will probably take a few years, but it doesn't require much work from the PureScript compiler, it's just a general community transition.

@wegry
Copy link

wegry commented Sep 29, 2016

Sorry for the wall of text.

a PureScript bundler will be able to perform optimizations that a JS bundler will never be able to do

@dimsol Correct me if I'm wrong, but the optimizations #1206 covered (undocumented elsewhere?) in 0.9.x psc-bundle currently are:

- Live module loading (only what's used)
- Tree shaking (which is more aggressive about output JS using things like `new` that have side effects)
- Decurrying

As covered in #1206, there doesn't seem to be enough there currently to warrant usage in projects that happen to mix JS and PS.

I understand that the PS community finds JS and its tooling as whole undesirable. I've just been pushing for not crowding out alternate implementations and want no part in that debate.

For us, at least, psa wrapping psc 0.8 sometimes takes 12 minutes or hangs for hours to do a full rebuild of 400 modules and 3 MB of CJS to 400 files. Webpack linting and transpiling 60 and loading 800 modules in 60 seconds is pretty much the cost of doing business. There's warts to go all around.

Decurrying is the only optimization my team currently dearly misses, because the other two issues are mostly covered by Webpack 2 + postprocessing. Decurrying could potentially be patched up with eval/apply instead of push/enter in psc-@ekmett might be able to answer questions there-but that's probably it's whole own issue.

The biggest drawbacks to using psc-bundle for us-also partially covered in #1206-were:

- Only supports CJS when we have dependencies with UMD, AMD, and legacy exports
- Leaking global variables
- No async loading of code of non-essential code, a la webpack's chunking or AMD loading
- Size and minification issues due to psc-bundle's storing modules as strings and export logic (40% reduction there)
- Tree shaking JS dependencies

psc's FFI export checker and artificial ES5 enforcement could be superceded by something like Webpack 2's ES module import checks which meet the static criteria laid out by @dimsol. I believe there was a discussion about dropping that, but as always, take that with a grain of salt.

In response to @Pauan's post:

1. Rollup's biggest flaw currently is that it only supports one module style (setting aside plugins for now). Webpack is currently exploring ways to pull ideas from Rollup. `psc` is in the same boat with CommonJS. It's fine until you want to use code from another module system.

Before my team used ES modules, we used AMD and obviously CJS for PS. To get our psc bundled code loaded, at runtime mind you, we had to wrap the whole legacy module psc-bundle exports in an AMD module.

My team, in an enterprise environment, doesn't control the entire page. So periodically, someone would change a dependency version and stomp on our library with a different version or API in the global namespace. If someone has to use a fallen JS dependency that hangs variables on the window (jQuery, Lodash, etc.), how does psc-bundle account for that?

Webpack allowed us to, at the very least, shield our internal dependencies from the global namespace-common libraries like jQuery and Lodash. In addition to bundling and minifing, we also use it for placing and versioning svgs, fonts, images, LESS, etc.

It's not really an apples to apples comparison between psc-bundle and webpack when those things are considered. Juche is fine but it's not free. And I'm not advocating breaking up the old pulp > psa > psc-bundle > psc toolchain other teams use. It's about being accomodating to teams that use alternate toolchains. We use webpack > psa > psc.

psc, on account of only supporting valid ES identifiers as of 0.9, doesn't have to change anything to use the loader I linked to very recently in #1206 for tree-shaking. No transpilation required @dimsmol.

This has the added benefit of as @Pauan mentioned:

Existing FFI code doesn't need to change

2. ES6 features will be nice as far as getting code to the browser goes. Things like object shorthand and arrows being the biggest bang for your buck. Release Firefox is at 94%, but Chrome, Edge, and Safari are all at 100% support outside of modules.

3. No comment ;)

@Pauan
Copy link

Pauan commented Sep 30, 2016

@wegry My proposal was about migrating all PureScript projects to ES6, regardless of whether they use Webpack, Rollup, psc-bundle, etc. Therefore it is very gentle and makes the minimal number of changes while retaining backwards compatibility.

I agree that Webpack is amazing when you want to seamlessly tie into the rest of the JS ecosystem, including all the wacky ES6/CommonJS/RequireJS/npm/global stuff. I also agree that PureScript should have good support for Webpack as an alternative (not replacement) to psc-bundle.

In my opinion, good support for alternative workflows (e.g. Webpack) is a separate (but very important!) issue. However, there may be some overlap between the two issues.


I tried out your Webpack loader, but I had to make some minor tweaks to get it to work right:

https://gist.github.com/Pauan/53b1af11a71d6ebac8defafad070b3fe

It's obviously very hacky, but it does work: in my testing, it was able to remove all of the require, exports, and module.exports from both PureScript and FFI code.

Despite that, Webpack2 still didn't do a very good job of tree shaking. From what I understand, Webpack2 doesn't do any actual treeshaking itself, instead it relies on UglifyJS to remove dead code. But UglifyJS is very conservative in the code that it removes, so the end result is a lot of bloat. In addition to that, Webpack2 still wraps each ES6 module in a separate function, which also prevents tree shaking.

I will try to write a Rollup plugin and then we can see if Rollup does a better job of tree shaking compared to Webpack2.

@Pauan
Copy link

Pauan commented Sep 30, 2016

Okay, I created a repository which tests the different ways of doing tree shaking with PureScript:

https://github.com/Pauan/purescript-tree-shaking

It compiles a program which is only slightly more complex than "Hello world". It imports JS files into PureScript, imports JS files into JS files, and imports PureScript files into JS.

I found that the best way to make it work is to use pulp build to output CommonJS files to the output folder and then afterwards slurp them up with Rollup/Webpack.

The purs-loader plugin works okay for Webpack, but I ran into some issues when trying to import an FFI file into another FFI file, so I decided to not use purs-loader. If I'm totally abusing the FFI and I'm not supposed to do that, please tell me.

If anybody has any suggestions for improvements, you can either tell me or submit a pull request.

P.S. This feels a bit off-topic, so perhaps we should move this discussion to a new issue?

Correctness

  • pulp-browserify fails to build, due to weird path issues when an FFI file requires another FFI file.
  • psc-bundle builds successfully, but fails at runtime because it doesn't support bundling require inside of FFI files.
  • webpack builds successfully, but fails at runtime with a bizarre error. In addition, the output looks really gnarly, so I suspect there is some sort of bug with the custom purs-loader.js
  • rollup builds successfully, and it works correctly at runtime too! In fact it's the only one that works correctly. It also produces very clean code.

Size

  • pulp-browserify can't build, so N/A
  • psc-bundle is 14.6 kB regular, 6.9 kB minified
  • rollup is 30.8 kB regular, 12.1 kB minified
  • webpack is 105.2 kB regular, 24.4 kB minified

I suspect that psc-bundle's dead code elimination is more aggressive than Rollup, probably due to purity.

Webpack's output is super bad, which makes me think that I'm doing something wrong. If anybody has any ideas, please do share.

@dimsmol
Copy link

dimsmol commented Sep 30, 2016

@wegry thanks for explanation!
I totally agree, making PureScript to play nice with third-party bundling (webpack or any other tools) is an important task.

Speaking about your particular case, as I understand, everything already works via not using psc-bundle and do all the bundling with webpack instead. Do you have any problems with your workflow that must be addressed in PureScript tooling?

I guess, in your scenario you could benefit from PureScript DCE if it could work on it's own without bundling. It still won't be at it's full power for JS code importing PureScript modules because in this case it works on module level, but it can remove a lot of stuff within PureScript code. Tracking which PureScript modules are required can be tricky, purs-loader actually does it incorrectly for now, but it can potentially be solved. If we could understand what exact parts of PureScript modules are required, PureScript DCE could work with all its power even for imports from JS code.

I propose to consider the following:

  • Make it possible to do DCE for PureScript without bundling (have separate modules as a result, but with all the dead code eliminated).
  • Allow to provide more fine-grained information for dead code eliminator - not only modules needed, but also particular exports required from those modules. With this feature it will be possible to improve, say, purs-loader to use PureScript DCE at it's max power.

For the drawbacks of psc-bundle you've mentioned, I totally agree that it would be nice to address the problems with global variables leakage and large module name strings. At the same time I don't think it's a task for psc-bundler to do JS tree-shaking or support wide variety of JS modules. It's better to use JS tools like webpack if you need such things. For async loading, it's a nice feature to have, but I'm ok with psc-bundler not having it (again, one can use webpack or some other JS bundler if really need it).

I like the proposition by @Pauan to incorporate ES6 modules support. In future we will need them anyway. I also like an idea to start using other ES6 features in compilation results, but not by default. Maybe it worth to have compilation flag(s) for this. Eventually we will need to use them all, but browsers and node.js must be ready first.

One more thing I want to address is psc's inability to understand non-CJS exports when checking FFI modules. I don't think there should be a support for wide variety of JS modules, but having support for ES6 would be nice because it's a future anyway. For the other kinds of modules the problem can be solved by making this check optional (allow to turn it off via compiler flag). The integrity in these cases must be checked by JS tooling (if possible).

The common idea is to make PureScript tools work best with the PureScript stuff and not care about JS specifics too much, but at the same time to not block the way for JS tools one may want to use over resulting JS code.

@garyb
Copy link
Member

garyb commented Jan 9, 2017

Trying https://jsperf.com/curried-vs-uncurried-body-overhead in

  • MacOS Chrome
  • MacOS Safari
  • MacOS Firefox
  • Windows Chrome
  • Windows Firefox

The duplicate body case was faster in every single case, except for Safari where it was a tie. Invoking functions has an overhead, tracing VM or not.

@garyb
Copy link
Member

garyb commented Jan 9, 2017

Some interesting information: when compiling my decent-sized PureScript application, 177 calls were curried, and 85 were uncurried. However, most of the curried calls were because typeclass instances aren't being inlined. If typeclass instances were inlined, I think that would swing things in the favor of uncurried calls.

I wanted to try the plugin on SlamData for a good real world example of how many curried vs uncurried functions there are, but unfortunately I wasn't able to. I spent about half an hour hacking case after case that it was complaining about but couldn't really afford to spend any more time on it. I'll open some issues when I get a chance. Mostly it was complaints about "invalid module" though, with a few cases of "duplicate export" (toString and hasOwnProperty are two cases of that that spring to mind, the former in -argonaut, the latter in -foreign).

@wegry
Copy link

wegry commented Jan 9, 2017

@garyb My opinion about the priority of perf work seems to be heterodox, and performance has historically been a sensitive issue. Please note I have no desire for a repeat of the tenor of #1206. That said, type-classes are indeed tremendously slow in our experience. The problem rears its head in JSON deserialization. We believed that inlining would save us a lot agony in that regard, but our pragma proposal looks to have died in committee.

The last time configuration came up, specifically concerning ES module exports and imports, it was shot down as fragmenting the build chain. That portion could easily be in psc (and @texastoland had already done the work at one point). It's the whole reason this particular transpiler was started in the first place. And here we are with a JS based optimizer pass.

I don't have a problem with that, but given that it's the edge case in how code will be called in purescript I don't think imposing a global penalty on the usual case is worth it.

Having blessed support of JS exports isn't a common use case because the blessed toolchain cannot currently support it. Both @Pauan and I have been arguing for better JS support here and elsewhere because many real world projects are not green field and have to accommodate JS. At the very least, it would help with adoption of the language.

And yes, if idiomatic exports aren't a thing still, I agree with you that the perf penalty of decurrying is not worth it.

The difference in uncurried v. curried perf at least in iOS Firefox was 3% (worth it in my opinion for idiomatic exports). On the desktop, I'm seeing a diff of 8%, which is significant. In the event that exports are not decurried, duplicating code or paying a >5% in runtime both seem to be unacceptable at least to me.

@Pauan
Copy link

Pauan commented Jan 9, 2017

@garyb Function calls have zero overhead when the function is optimized/traced/inlined/etc. Benchmarking and performance is hard, especially with advanced JIT engines.

When I said "extensive benchmarking" I mean benchmarks of actual programs, not microbenchmarks (which are unreliable for predicting application performance).

Please do file bug reports.

@rvion
Copy link
Contributor

rvion commented Jan 9, 2017

I think I would slightly prefer de-duplication as a default, and code duplication (inlining) as an option:

some arguments I can think of are

  • easier to debug, manually tweak
  • easier to set breakpoints, etc
  • easier to generate source-maps
  • easier to profile: no randomly splited cost-centers
  • smaller file to generate/write/serve when developing
  • easier to parse output
  • clearer separation between currying related stuff and logic

However, most of the curried calls were because typeclass instances aren't being inlined. If typeclass instances were inlined, I think that would swing things in the favor of uncurried calls.

That would really be great !

@paf31
Copy link
Contributor

paf31 commented Jan 9, 2017

I don't have time to post a full reply right now, but let me just summarize: I'm not thinking about optimization yet, but I plan to think about it seriously after 1.0. Right now, I'm focusing on 1.0, and anything not already prioritized will be put in the "Approved" milestone at best, unless there is a really good reason for it to go into 1.0.

For 1.0, I would prefer that we cut DCE from psc-bundle if it can be replaced by external tools. I've created an issue to track that. But that's not to say that we can't return to DCE in the compiler later, if it can be done properly, and in a way which is compatible with ES6 without hacks.

I have no problem with supporting ES6 modules before 1.0, but I don't want to support two modes of code generation, and we would need to come up with a list of work to be done, since a change like that would mean changes to psc, Pulp, purs-loader, etc.

@michaelficarra
Copy link
Contributor Author

@paf31 I just wanted to add that I think it would be a huge shame if we released 1.0 while generating CommonJS-based modules instead of real ECMAScript modules. In my opinion, it is an absolute must for the 1.0 release.

@paf31
Copy link
Contributor

paf31 commented Jan 10, 2017

I agree, and if we cut features from psc-bundle at the same time, then there are no concerns with language-javascript any more, right?

@Pauan
Copy link

Pauan commented Jan 10, 2017

I would just like to point out that rollup-plugin-purs is an excellent way to transition toward ES6 modules: it works by converting CommonJS to ES6 modules, and it gives warnings if you use the dynamic features of CommonJS (which are incompatible with ES6 modules).

So by compiling your PureScript library/application with rollup-plugin-purs you'll be able to see which parts of your library are incompatible with ES6, that way you can fix those parts ahead of time.

In fact, it's possible to write a program which will take in CommonJS source code and spit back ES6 source code (which is what rollup-plugin-purs is already doing), which will help to automate the transition to ES6.


@paf31 From what I understand, the only reason for language-javascript is to ensure that the FFI files contain the appropriate exports, correct? But Rollup already verifies that the exports are correct (for all JS files), so I don't think it's necessary for psc to do those checks.

@hdgarrood
Copy link
Contributor

hdgarrood commented Jan 10, 2017

@Pauan Inside psc, yes, that's the only reason we use language-javascript. Of course psc-bundle uses the JS AST for dead code elimination too, but I guess we already have an answer for that :)

I think I'd be happy to drop those checks from psc, provided that the errors that Rollup gives you in a case where you foreign import something which you forgot to define in the relevant FFI module are reasonably easy to understand.

@Pauan
Copy link

Pauan commented Jan 10, 2017

@hdgarrood Here is the error that psc gives:

Error found:

  The following values are not defined in the foreign module for module Pauan.View:

    mapImpl



See https://github.com/purescript/documentation/blob/master/errors/MissingFFIImplementations.md for more information,
or to contribute content related to this error.

Here is the warning that Rollup gives:

⚠️   'mapImpl' is not exported by 'output/Pauan.View/foreign.js'
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
output/Pauan.View/index.js (20:52)
18:     return Data_Functor.map(Pauan_Stream.functorStream)(Pauan_StreamArray_Class.Replace.create)(Pauan_Stream.stream(toStreamView)(view1));
19: });
20: var functorView = new Data_Functor.Functor($foreign.mapImpl);
                                                        ^
21: var applyView = new Control_Apply.Apply(function () {
22:     return functorView;

It's a lot messier (because the message includes a few lines of actual source code), but it does tell you the name of the function (mapImpl), and which module it came from (Pauan.View)

@hdgarrood
Copy link
Contributor

Nice :)

@Pauan
Copy link

Pauan commented Jan 10, 2017

I just pushed out version 1.0.4 of rollup-plugin-purs, which includes a convert-commonjs command which you can use to convert a CommonJS file to an ES6 module. You use it like this:

node_modules/.bin/convert-commonjs foo.js > foo.js

This will replace the CommonJS file foo.js with an ES6 file foo.js. This can be used to automate the conversion process from CommonJS to ES6 (but you might need to manually tweak the result). Make backups before you convert (Git counts as a backup).

@bodil
Copy link

bodil commented Jan 10, 2017

While I'd obvs much rather see the compiler itself output ES6 natively, I'd say it's time to start playing around with integrating rollup-plugin-purs into Pulp. It could be a good place to de-complicate those JS specific error messages too.

@Pauan
Copy link

Pauan commented Jan 10, 2017

@bodil Of course, but there will be a transition period where PureScript libraries need to convert their FFI files from CommonJS to ES6, that's what convert-commonjs is for.

Also, as a side note, rollup-plugin-purs should work fine with either CommonJS or ES6, or even a mixture of different module systems. So that will help out a lot during the transition.

@garyb
Copy link
Member

garyb commented Jan 10, 2017

@Pauan I've just opened a selection of issues that I encounter when trying to use the plugin on SlamData, although I've only just realised some of them are warnings only, and shouldn't block compilation (I think?). On Windows I just see squiggle-boxes rather than the icons or whatever it is that is supposed to be next to the errors, so it's not easy to tell which are fatal.

@Pauan
Copy link

Pauan commented Jan 10, 2017

@garyb Thanks, I'll address those now. If the compilation succeeded, then they were warnings, if the compilation failed then it was an error. Rollup seems to use ⚠️ for warnings and 🚨 for errors.

@garyb
Copy link
Member

garyb commented Jan 10, 2017

@Pauan that's why I can't tell, I never got the compilation to succeed 😉 and yeah, those do not show up as they're supposed to:

squiggles

I'm assuming that it halts on a fatal error now though, so won't open issues for anything above the last message that I encounter in the future (unless the warning seems incorrect somehow).

@Pauan
Copy link

Pauan commented Jan 10, 2017

@garyb Ah yeah, I can see how that's confusing. I'm pretty sure that fatal errors cause it to immediately stop, so anything other than the bottom-most error is a warning.

@wegry
Copy link

wegry commented Jan 10, 2017

@garyb Would you be able to paste the output that generates the dynamic require warning? Webpack has a similar constraint on static and verifiable imports (and exports). If Debug.Trace uses a CJS import and then attempts to modify it you could be in for a rough patch trying to move that over to ES Modules. This is something our code base ailed from moving from AMD and CJS to exclusively ES Modules. Your export names adversely affect this approach as well.

One of the major pitfalls of transpilation as a post processing step as opposed to being baked into psc is that name mangling is pretty tricky. With our internal transpilation method (which has been in use for some time now) we recently found a bug where exports that would be invalid ES6+ were affecting object properties of the same name (default). Because the current mangler (internally, and yet to be reconciled with @Pauan's pending go ahead from @gbaz) uses a regex to look for object property accessors it picks up plain Jane objects with that reserved key as well.

e.g.

Made_Up_Module["default"] becoming Made_Up_Module.purescript_default breaks on

var foo = {
  default: 'oh yeah'
}

foo.default

becoming

var foo = {
  default: 'oh yeah'
}

foo.purescript_default

Edit to include the link to the issue @garyb mentioned.

@garyb
Copy link
Member

garyb commented Jan 10, 2017

@wegry see the issues on the plugin repo. Those dynamic things are just warnings, so it's fine - they occur in a couple of modules when there is behaviour that needs patching so a module works in node as well as the browser.

@Pauan
Copy link

Pauan commented Jan 12, 2017

@wegry This is the warning with the dynamic require, but it's not actually a problem, it should work fine with rollup-plugin-purs

There is a more serious issue with virtual-dom, but rollup-plugin-purs was never intended to support arbitrary CommonJS modules, only PureScript code. I think the best way forward is to send a pull request to @Raynos which fixes the export issue. It's a really easy fix.

rollup-plugin-purs doesn't have any problems with importing/exporting reserved words. And it uses a full parse + AST (not regexps), so it doesn't mangle arbitrary objects, so that's not a problem either. In general rollup-plugin-purs should be quite solid.

@hdgarrood
Copy link
Contributor

hdgarrood commented Jan 15, 2017

I think it might be time to close this and split it up into individual issues, as it's drifted a long way from the original proposal. We already have #2545; are there any other items that we need to track?

edit: I suppose we will want an issue for generating ES6 modules instead of CommonJS in psc, although we're not quite ready to actually start doing that yet.

@Pauan
Copy link

Pauan commented Jan 15, 2017

@hdgarrood I created #2574 and #2575

@hdgarrood
Copy link
Contributor

I think it's probably safe to assume that covers it then, so I'm going to close this. Please do add a comment if there's anything else I've missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests