Better runtime code

[smorimoto]

Not only for style consistency, but also add semicolons and other things that are theoretically better to have.

[smorimoto]

This allows external compilers such as esbuild to know which parts of the code can be optimised, so they can optimise more aggressively.

[hhugo]

This allows external compilers such as esbuild to know which parts of the code can be optimised, so they can optimise more aggressively.

I don't really see the connection with this PR. Can you elaborate?

[hhugo]

The PR introduce a bunch of es6 features. It would be nice to understand what that mean for compatibility

[hhugo]

Mixing formatting and other changes is not ideal for reviewing

[smorimoto]

I just split the format commit to another PR: #1673

[smorimoto]

I don't really see the connection with this PR. Can you elaborate?

The scope is different for var and const/let.

The PR introduce a bunch of es6 features. It would be nice to understand what that mean for compatibility

The ES6 features used here are supported by all major browsers except IE11, and stable versions of Node.js up to 3 generations ago, and should Just Work™ without transpiling.

https://compat-table.github.io/compat-table/es6

[TyOverby]

We use Js_of_ocaml to compile OCaml down to Google Apps Script which has a modern javascript runtime, but it validates the code using (I think) an ES3 parser. We've had issues in the past where using modern features in the runtime.js files will break these app-script scripts.

[TyOverby]

Also, could we avoid making big formatting changes until wasm_of_ocaml is merged?

[hhugo]

Also, could we avoid making big formatting changes until wasm_of_ocaml is merged?

Can you explain the reasoning here ?

[hhugo]

Until 2020, Google Apps Script was based on Mozilla's Rhino JavaScript (JS) interpreter, which limited its JS language support to version 1.6, with a subset of the ECMAScript 5 API.[5] In March 2020, Google announced the introduction of the V8 JS runtime, bringing with it full support of modern ECMAScript except for JS modules.[6]

@TyOverby, it seems that apps script should not be an issue anymore. Can you check and report back. It would be nice to be able to modernize the runtime a bit.

[TyOverby]

A few months ago we had to remove bigint literals from a runtime file because it wasn’t supported, so if they are using v8, it’s a pretty old v8

[hhugo]

A few months ago we had to remove bigint literals from a runtime file because it wasn’t supported, so if they are using v8, it’s a pretty old v8

Can you ask the people using app scripts to report what's supported ?

[smorimoto]

Ready for review...

[TyOverby]

I originally missed the page which says that the migration to v8 happens automatically for some files, but not for others, so I'll check with those teams to see if they just didn't move over yet.

A few months ago we had to remove bigint literals from a runtime file because it wasn’t supported, so if they are using v8, it’s a pretty old v8

Can you ask the people using app scripts to report what's supported?

I can ask, but the maintainers of those scripts probably don't know more than I do, and are less equipped to test a bunch of language features. I'll hold off on doing this until I can confirm that they did in fact move their scripts to v8.

[TyOverby]

Ok, I did some random testing, and the only modern feature (that I could find) that they don't support is bignum literals... The changes made in this PR seem fine

[hhugo]

Ok, I did some random testing, and the only modern feature (that I could find) that they don't support is bignum literals... The changes made in this PR seem fine

Did you actually try to run app scripts against this PR ?

[hhugo]

@smorimoto, I've been looking at this a bit today. I think I would be more confortable reviewing such change piece by piece. One PR at a time.
Can we start with a config that make the CI pass and reenable passes one at a time ?

The following seems to make the lint happy

  "linter": {
    "enabled": true,
    "rules": {
      "recommended": true,
      "style": {
        "noVar": "off",
        "noParameterAssign": "off",
        "noUselessElse": "off",
        "useTemplate": "off",
        "useSingleVarDeclarator": "off",
        "useWhile": "off",
        "noCommaOperator": "off",
        "noArguments": "off",
        "useNumberNamespace": "off",
        "useExponentiationOperator": "off",
        "useNodejsImportProtocol": "off"
      },
      "suspicious": {
        "noRedeclare": "off",
        "noDoubleEquals": "off",
        "noAssignInExpressions": "off",
        "noSelfCompare": "off",
        "noFallthroughSwitchClause": "off",
        "noGlobalIsNan": "off",
        "noGlobalIsFinite": "off",
        "useDefaultSwitchClauseLast": "off",
        "noControlCharactersInRegex": "off",
        "noRedundantUseStrict": "off",
        "noConfusingLabels": "off",
        "noDuplicateCase": "off"
      },
      "correctness": {
        "noInnerDeclarations": "off",
        "noSwitchDeclarations":"off",
        "noInvalidUseBeforeDeclaration": "off"
      },
      "complexity": {
        "useArrowFunction": "off",
        "useOptionalChain": "off",
        "noUselessSwitchCase": "off",
        "useLiteralKeys":"off",
        "noUselessTernary": "off",
        "noExtraBooleanCast": "off"
      },
      "security": {
        "noGlobalEval": "off"
      }
    }
  },

We could then start by fixing things like, noExtraBooleanCast, noDuplicateCase, noConfusingLabels, noRedundantUseStrict for example.

Checks that we want to keep off should be annotated with a reason (can we have comments ?)

[smorimoto]

@hhugo This PR is already fully completed and I believe no additional changes will be made. However, if you still prefer to split this up, I think I can split this PR into smaller pieces. (In other words, I'm trying to avoid it because it's not a very easy route 😅)

[TyOverby]

Ok, I did some random testing, and the only modern feature (that I could find) that they don't support is bignum literals... The changes made in this PR seem fine

Did you actually try to run app scripts against this PR ?

No, and I won't be able to until wasm_of_ocaml is merged

[hhugo]

@hhugo This PR is already fully completed and I believe no additional changes will be made. However, if you still prefer to split this up, I think I can split this PR into smaller pieces. (In other words, I'm trying to avoid it because it's not a very easy route 😅)

@smorimoto, as maybe shown in #1677, the changes in this PR are not as trivial as they may look at first.

• I'm still unsure about the es6 changes arrow, let, ..
• The suspicious and correction kind of linting probably deserve their own PR to reduce the noise while reviewing
• The style kind, we can probably leave for later. but maybe noUselessElse and noCommaOperator
• ...

[smorimoto]

Makes sense

[hhugo]

Ok, I did some random testing, and the only modern feature (that I could find) that they don't support is bignum literals... The changes made in this PR seem fine

Did you actually try to run app scripts against this PR ?

No, and I won't be able to until wasm_of_ocaml is merged

Similar to how janestreet/ppx_expect#54 is held back, I find the situation unpleasant..