Proposal: Remove `language-javascript` · Issue #4227 · purescript/purescript · GitHub
Skip to content

Proposal: Remove language-javascript #4227

Open
Open
Proposal: Remove language-javascript#4227
Labels
purs bundletype: breaking changetype: enhancement
Milestone
v0.16.0

Description

@thomashoneyman
thomashoneyman
opened on Jan 16, 2022

Summary

In #4226 I proposed that we remove the PureScript bundler. If we remove the bundler, then we only use language-javascript to verify that FFI declarations have a corresponding export in a .js file of the same name as the PureScript module. This library has been a notable maintenance burden on us over the past few years and I would like to move away from it.

Therefore, I propose that we drop the language-javascript dependency altogether if & when we remove the PureScript bundler.

To do this, we would need to do one of:

  • No longer verify FFI declarations have a corresponding .js export, instead relying on the immediate errors that would happen on application load if there was no corresponding export
  • Write our own minimal JavaScript lexer / parser that only looks for top-level declarations and exports for the purposes of validating FFI, but which doesn't attempt to parse all of the JavaScript.

Motivation

We've had trouble with language-javascript over the past few years. We currently pin an old version to avoid a bug parsing the async keyword, and we've supplied the majority of pull requests to the library since 2019.

While a good library, it has plenty of bugs in it and it's become a maintenance burden for the compiler team. Removing language-javascript allows us to drop this burden.

Proposal

I am proposing that we remove language-javascript if & when we remove purs bundle.

Without purs bundle, the only reason to have a JavaScript parser is to ensure that FFI declarations have a corresponding implementation. This leaves us with a decision:

  1. No longer verify this, leaving it to be a runtime or bundler error
  2. Write a "simple" JavaScript lexer / parser that only cares about exports

In the first case there is really nothing for us to do besides removing the dependency and corresponding code.

In the second case, we would need to write our own code. This code would need to be able to lex JavaScript literals like strings, brackets, export declarations, identifiers & keywords, and comments, so as to parse out exports. Everything else could be skipped over.

I think we should do (2) and maintain our own simple parser. I think this parser should only support "common sense" JavaScript rather than try to cover all edge cases. FFI files are not "do what you want" JavaScript, after all: they're a bridge between the PureScript and JavaScript worlds, and so I think we can place some restrictions on the JavaScript to make it easier for us to parse the export declarations.

It's not necessary to drop language-javascript in order to drop purs bundle, but dropping purs bundle opens the door to drop language-javascript as well.

Examples

Since this is the removal of a feature rather than an addition, I don't have any examples. However, I'm happy to edit this proposal to add them if y'all have ideas on what you'd like to see!

Activity

  1. thomashoneyman
    added
    type: enhancement
    purs bundle
    affects-v0.15.0
    on Jan 16, 2022

  2. JordanMartinez commented on Jan 17, 2022

    @JordanMartinez
    JordanMartinez
    on Jan 17, 2022
    Contributor

    If purs bundle is removed, I would say dropping language-javascript and not reimplementing the FFI check initially is what we should do. In other words, I think dropping the FFI check initially and then adding that feature back in later would be a wise strategy. Supporting ES modules will already take a lot of work, so this will make it easier to make that transition.

  3. sigma-andex commented on Jan 17, 2022

    @sigma-andex
    sigma-andex
    on Jan 17, 2022
    Contributor

    As per my comment here I think esbuild is at least able to provide this simple sanity check when bundling.

  4. hdgarrood commented on Jan 17, 2022

    @hdgarrood
    hdgarrood
    on Jan 17, 2022
    Contributor

    There is a downside to option 1 which I didn’t previously identify, which is that the current check can catch extraneous exports - if the FFI module exports something which the corresponding PureScript module does not foreign import, it’s nice to be warned about that because it’s usually a mistake, and I imagine JS bundlers probably won’t tell you if this is the case. That said, I do prefer option 1 here.

    I think this parser should only support "common sense" JavaScript rather than try to cover all edge cases. FFI files are not "do what you want" JavaScript, after all: they're a bridge between the PureScript and JavaScript worlds, and so I think we can place some restrictions on the JavaScript to make it easier for us to parse the export declarations.

    This approach makes me a little uneasy, because I think most of the problems people have with purs bundle can be traced back to the fact that we previously took this exact approach. These problems are made worse by the fact that we never documented exactly what we meant by “common sense JavaScript,” which meant that it was usually impossible to answer questions about why purs bundle was misbehaving without referring to the implementation, which sucks. I think we should only roll our own incomplete JS parser if we intend to document exactly what we mean by “common sense JavaScript,” because it certainly won’t be obvious where the boundaries are otherwise (eg. is it “common sense” to allow omitting semicolons?)

    I also think we should consider depending on a better maintained JS parser if we really don’t want to lose the ability to parse FFI modules. For example, tree-sitter is probably a decent option, if a bit low-level. I did previously object to using tree-sitter, but I think having purs bundle out of the picture does simplify the matter a lot and I also think I underestimated the value of not having to fix upstream bugs ourselves.

  5. rhendric commented on Jan 18, 2022

    @rhendric
    rhendric
    on Jan 18, 2022
    Member

    Option 1 makes the most sense to me; I don't have anything to add that @JordanMartinez and @hdgarrood haven't already said well.

  6. sigma-andex commented on Jan 18, 2022

    @sigma-andex
    sigma-andex
    on Jan 18, 2022
    Contributor

    There is a downside to option 1 which I didn’t previously identify, which is that the current check can catch extraneous exports - if the FFI module exports something which the corresponding PureScript module does not foreign import, it’s nice to be warned about that because it’s usually a mistake, and I imagine JS bundlers probably won’t tell you if this is the case. That said, I do prefer option 1 here.

    Is that really a problem? To me it looks more like a nice-to-have when I forgot to remove an FFI export that I don't use anymore rather than something that can cause issues.
    Also, I checked it with esbuild and it will remove the unused export when bundling.

    E.g. in my demo the foreign.js exports `someoneForgotToRemoveMe` 
    export { helloFromCJS } from "example-cjs";
export { helloFromESM } from "example-esm";
import { helloFromPSESM } from "example-ps-esm"
export const helloFromPSESMviaFFI = helloFromPSESM
export const someoneForgotToRemoveMe = "Unused"
    in the index.js it is not re-exported 
    // Generated by purs version 0.15.0
import * as $foreign from "./foreign.js";
import * as Effect_Class from "../Effect.Class/index.js";
import * as Effect_Class_Console from "../Effect.Class.Console/index.js";
import * as Example_ESM from "../Example.ESM/index.js";
var main = /* @__PURE__ */(function __do() {
    Effect_Class_Console.log(Effect_Class.monadEffectEffect)("Hello from my project")();
    Effect_Class_Console.log(Effect_Class.monadEffectEffect)($foreign.helloFromCJS)();
    Effect_Class_Console.log(Effect_Class.monadEffectEffect)($foreign.helloFromESM)();
    Effect_Class_Console.log(Effect_Class.monadEffectEffect)(Example_ESM.helloFromPSESM)();
    return Effect_Class_Console.log(Effect_Class.monadEffectEffect)($foreign.helloFromPSESMviaFFI + " via FFI")();
});
export {
    helloFromCJS,
    helloFromESM,
    helloFromPSESMviaFFI
} from "./foreign.js";
export {
    main
};
    and esbuild bundle automatically removes it.

  7. hdgarrood commented on Jan 18, 2022

    @hdgarrood
    hdgarrood
    on Jan 18, 2022
    Contributor

    It’s not a major problem but it is nice to have, for the same reason that the warning for unused bindings is nice to have. From time to time it can prevent people from being genuinely misled in a way that can lead to bugs or at least wasted time, I think.

  8. JordanMartinez commented on Apr 22, 2022

    @JordanMartinez
    JordanMartinez
    on Apr 22, 2022
    Contributor

    I believe this issue will be resolved in a future 0.16.0 release.

  9. JordanMartinez
    added this to the v0.16.0 milestone on Apr 22, 2022
  10. JordanMartinez
    removed
    affects-v0.15.0
    on Apr 22, 2022

  11. hdgarrood commented on Apr 22, 2022

    @hdgarrood
    hdgarrood
    on Apr 22, 2022
    Contributor

    Just out of interest - the original issue proposes dropping language-javascript if and when we drop purs bundle. We are dropping purs bundle in 0.15 right? If so, why not drop language-javascript at the same time?

  12. JordanMartinez commented on Apr 22, 2022

    @JordanMartinez
    JordanMartinez
    on Apr 22, 2022
    Contributor

    IIRC, language-javascript is not being dropped because it'll be used to fail the build if it successfully parses the file and CJS is detected. If it isn't able to parse the file, no error or warning is emitted.

  13. JordanMartinez
    added
    type: breaking change
    on Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    purs bundletype: breaking changetype: enhancement

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions