Warn when encountering ES6+ syntax in FFI modules

[milesfrain]

Should the registry have a policy on banning ES6 syntax?

A check for this could be incorporated into CI, as is done for some libraries already.

Another option is to require an "ES6-override" flag in Package.dhall - indicating that the author is aware that ES6 syntax is used. Then packages without this flag would not be allowed to depend on packages with this flag. Another pursuit feature (#1) could be to note the status of this flag.

[hdgarrood]

👍 I would definitely be in favour of banning ES6 syntax (apart from modules, once we implement those, of course).

[hdgarrood]

I do think it's reasonable to ban it entirely rather than allowing it to be opt-in via a flag. People can always fetch dependencies from elsewhere if they don't want to comply, or alternatively they can compile their FFI to ES5 locally before publishing.

[f-f]

Agreed with both, I'll add this to the list in purescript/registry#23

[f-f]

How would we implement this check?

[hdgarrood]

I suspect the easiest way of doing it would be to use a JS linting tool like eslint. Implementing “ES5 except that it should use ES modules” might be a bit more complicated; I’ll try to see if I can put a configuration together that does this.

[hdgarrood]

Actually eslint might not work so well, as rules can be disabled with inline comments.

[hdgarrood]

The compiler already parses FFI files in order to check that the exported values on the JS side match with the imported values on the PureScript side. Perhaps it would make sense to add this check inside the compiler, and emit a warning when encountering ES6+ syntax? Then it ought to be relatively easy to turn that specific warning into an error in our CI checks pipeline.

[f-f]

@hdgarrood should we then move this issue to the compiler repo?

[hdgarrood]

Yep, sounds good.

[garyb]

I think an opt-in would be nice to have. Disallowing ES6-using projects from the registry entirely seems rather proscriptive about something that is basically an opinion - it's not any less safe than other JS or anything like that, it's just that it might cause dependents to require another build step in the project depending on target and such. If there's a flag that they have to turn on to say "yes, I know" then it won't trip people up unexpectedly at least.

I guess one positive of that I can see is it would head off any per-library arguments about ES6, since I could imagine the case where there's an ES6 library published, someone wants to use it but notices it's only using arrow functions in two places or something, opens a PR to "fix" it, and then a stylistic holy war ensues.

Regarding it in the compiler, would this warning only apply specifically to module-linked FFI files? Because it could be that the FFI is ES5, but it's requireing something that is ES6. What happens then?

[chekoopa]

Please, make up your minds, as a #3613 comment tells that ES modules are presumably pre-rolling on 0.14 and rolled out on 0.15. Considering there's a PR already implementing them.

BTW, is it possible to keep compatibility both with CJS and ES?

[hdgarrood]

ES modules are definitely happening and are unrelated to this issue. There is no contradiction: when we talk about "banning ES6 syntax" we only mean for things which already have a perfectly serviceable ES5 equivalent, like function expressions. The purpose is to ensure that after bundling, code is likely to run in as many JS environments as possible. The module syntax is irrelevant to that, because you need to do some kind of bundling anyway. What's important is that the output of purs bundle is ES5, which it will still be after we implement ES modules. Please let's not discuss ES modules any further on this issue.

[rhendric]

I don't have a very strong opinion here but I don't think I understand the advantage to artificially restricting this. If some bit of foreign code needs to use ES6 classes to interop correctly with a library (you can't extend an ES6 class with pre-ES6 prototypal inheritance patterns (well you can sort of approximate it but only if the base class has no constructor)), would that be allowed? If so, the supported targets argument is moot (it's mostly moot already for many developers; IE11 is pretty much the last place that ES6 isn't supported natively). If not, that's an unfortunate limitation and an opt-out at least would be nice.

[hdgarrood]

My motivation was essentially that it would be pretty irritating to have your site break for 1% of your users, and then have to set up Babel or something, just because one of your transitive dependencies was written by someone who preferred writing x => ... to function(x) { return ... }. You do raise a good point about classes, I wasn't aware of that one.

[rhendric]

Requiring that foreign modules be pure ES5 doesn't even guarantee that every PureScript program will work for that last percentile of browser users without further processing, since non-PureScript transitive dependencies could also screw things up for you. Without incorporating Babel into PureScript, or doing some sort of deep dependency linting, I don't think we can actually provide that guarantee. In the absence of a guarantee, anyone who really wants to be sure that those pesky IE holdouts are going to have a good experience with their PureScript app is going to have to use Babel anyway, unless they inspect all their foreign dependencies every time they upgrade.

So the tradeoff is, what, going from ‘some (or perhaps most?) PureScript programs will break for one percent (and falling) of browser users; an additional build step fixes it’ to ‘rather fewer, but not zero, PureScript programs will break for one percent (and falling) of browser users; an additional build step fixes it’, in exchange for everyone, including people who don't care about IE, or even browsers, giving up some tricky-to-desugar JS syntax (sticky regexes are a favorite of mine when doing parsing; generators can also simplify many problems) and some percentage (probably quite a small percentage, but still) of foreign JavaScript libraries that can't be used without ES6 features in the calling code? And that additional build step is already basically assumed to be necessary in the wider JavaScript ecosystem to reach the last percentile of users anyway? I just don't know if that's worth it in the end. (To be clear: maybe it is—as I said, I don't have a strong opinion. But the question should be asked.)

[hdgarrood]

Well, of course we aren't ever going to be able to guarantee that things work for everyone; as is usually the case, we are just trying to address specific, avoidable problems.

anyone who really wants to be sure that those pesky IE holdouts are going to have a good experience with their PureScript app is going to have to use Babel anyway, unless they inspect all their foreign dependencies every time they upgrade.

This is true, but it's also worth noting that it's completely possible to:

• have no foreign dependencies at all, or
• to obtain all of your foreign dependencies in a compiled (e.g. from ES6+ to ES5) form, so that they do work pretty much everywhere.

Try PureScript fits into this category, for example: right now it depends only on underscore, ace, and jquery, all of which it obtains from a CDN. It works in IE11, despite not involving Babel, because we avoid unnecessary ES6+ syntax in the PureScript libraries we pull in.

The general goal I would like to achieve is that, in the absence of foreign dependencies, the output of purs compile followed by purs bundle should not need additional processing to be deployable. Just because some additional build step is basically assumed to be necessary in the wider ecosystem, I don't think it's safe for us to make that same assumption. I'm sure there are tons of projects out there which don't involve npm, webpack, parcel, etc, (all of which have been around for only a relatively small period compared to JS itself). In fact, I do know of at least a few PureScript users who highly appreciate the fact that they're not required to install JS tooling to get things working.

Anyway, I'm persuaded that we shouldn't disallow ES6+ in FFI modules in the registry. I would still like to disallow ES6+ in the core libraries and it would be handy to have tooling support for that, but maybe it's not worth the effort.

[hdgarrood]

If this isn't going to be a requirement for submitting to the registry, perhaps we should close? Generally the compiler shouldn't care very much about the contents of FFI modules, and I think if requiring ES5 syntax is going to be something library authors opt in to doing, it's probably easiest to do it with a JS linter.

[gabejohnson]

It's important to keep in mind that language-javascript has some outstanding bugs with regards ES6+ syntax:

erikd/language-javascript#115
erikd/language-javascript#116

So if the compiler is going to rely on language-javascript it's not safe to allow ES6+ syntax generally in FFI generally.

[gabejohnson]

There are actually more unresolved issue than the ones I listed: https://github.com/erikd/language-javascript/issues

[hdgarrood]

I am at least aware of the async one - right now we are on an older version of language-javascript which isn't affected by that bug, and I think we should not upgrade until it is fixed. In general though, I think most language-javascript bugs aren't too big a deal: if something parses when it shouldn't, that will likely be caught in the browser as soon as you try to run the code, and if something doesn't parse when it should, there is usually a workaround (and if there isn't and the maintainer is unresponsive, we can always fork).

[gabejohnson]

if something parses when it shouldn't, that will likely be caught in the browser as soon as you try to run the code, and if something doesn't parse when it should, there is usually a workaround...

I'm more concerned with the case where something parses into the wrong AST and results in valid (though incorrect) code being generated during bundling.

and if there isn't and the maintainer is unresponsive, we can always fork

This appears to be the case, currently.

[hdgarrood]

That's a good point, that is more problematic. And the function expression issue you submitted a PR for does seem like something which could introduce failures at runtime. We'll have to revisit this when we're closer to making the ES modules changes, I guess.

[milesfrain]

I'm persuaded that we shouldn't disallow ES6+ in FFI modules in the registry.
If this isn't going to be a requirement for submitting to the registry, perhaps we should close?

Sounds good.