Fix TypeScript lint errors #6567

somebody1234 · 2023-05-04T17:19:54Z

Pull Request Description

Should fix TypeScript lint errors.
These errors do not show up locally as the imports point to built assets.

Important Notes

None

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

somebody1234 · 2023-05-04T17:20:52Z

opening as draft to trigger a CI run to confirm whether the fixes actually work

somebody1234 · 2023-05-04T17:26:06Z

not quite sure whether this actually works. lint is green, however it's entirely possible it's because this runner already has the relevant build artifacts from a previous run. (i don't know how exactly the CI runners work around here)

somebody1234 · 2023-05-04T17:30:52Z

This PR does make this change for consistency:
To make sure that it does the same thing, I'm running ide watch which imports content-config via content/index.ts. To test it I think making sure option parsing still works should be sufficient.

-import * as linkedDist from '../../../../../target/ensogl-pack/linked-dist/index'
+import * as linkedDist from '../../../../../target/ensogl-pack/linked-dist'

enso/app/ide-desktop/lib/content-config/src/index.ts

Line 5 in 27bbfe1

import * as linkedDist from '../../../../../target/ensogl-pack/linked-dist'

somebody1234 · 2023-05-04T17:31:17Z

status update: wasm failed to build with the classic:

 INFO ide_ci::program::command: npx⚠️ ▲ [WARNING] "import.meta" is not available with the "cjs" output format and will be empty [empty-import-meta]
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     pkg.js:4214:39:
 INFO ide_ci::program::command: npx⚠️       4214 │         input = new URL('pkg_bg.wasm', import.meta.url);
 INFO ide_ci::program::command: npx⚠️            ╵                                        ~~~~~~~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️   You need to set the output format to "esm" for "import.meta" to work correctly.
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️ ✘ [ERROR] Could not resolve "env"
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     pkg.js:16:29:
 INFO ide_ci::program::command: npx⚠️       16 │ import * as __wbg_star3 from 'env';
 INFO ide_ci::program::command: npx⚠️          ╵                              ~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️   You can mark the path "env" as external to exclude it from the bundle, which will remove this error.
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️ ▲ [WARNING] The CommonJS "exports" variable is treated as a global variable in an ECMAScript module and may not work as expected [commonjs-variable-in-esm]
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     snippets/ensogl-text-msdf-a35c812d6b3d67d9/msdfgen_wasm.js:1:940548:
 INFO ide_ci::program::command: npx⚠️       1 │ ...f(typeof exports==="object"){exports.ccall=ccall;exports.getValu...
 INFO ide_ci::program::command: npx⚠️         ╵                                 ~~~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️   This file is considered to be an ECMAScript module because of the "export" keyword here:
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     snippets/ensogl-text-msdf-a35c812d6b3d67d9/msdfgen_wasm.js:2:2:
 INFO ide_ci::program::command: npx⚠️       2 │ ; export { ccall, getValue, _msdfgen_getKerning, _msdfgen_setVariat...
 INFO ide_ci::program::command: npx⚠️         ╵   ~~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️ 2 warnings and 1 error

somebody1234 · 2023-05-04T17:42:56Z

mini self QA (highly recommend for someone else to do a real QA):
✔️ ./run watch + ide
✔️ ./run watch + dashboard
✔️ npm run typecheck
✔️ npx eslint .

wdanilo · 2023-05-04T18:04:41Z

status update: wasm failed to build with the classic:

 INFO ide_ci::program::command: npx⚠️ ▲ [WARNING] "import.meta" is not available with the "cjs" output format and will be empty [empty-import-meta]
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     pkg.js:4214:39:
 INFO ide_ci::program::command: npx⚠️       4214 │         input = new URL('pkg_bg.wasm', import.meta.url);
 INFO ide_ci::program::command: npx⚠️            ╵                                        ~~~~~~~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️   You need to set the output format to "esm" for "import.meta" to work correctly.
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️ ✘ [ERROR] Could not resolve "env"
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     pkg.js:16:29:
 INFO ide_ci::program::command: npx⚠️       16 │ import * as __wbg_star3 from 'env';
 INFO ide_ci::program::command: npx⚠️          ╵                              ~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️   You can mark the path "env" as external to exclude it from the bundle, which will remove this error.
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️ ▲ [WARNING] The CommonJS "exports" variable is treated as a global variable in an ECMAScript module and may not work as expected [commonjs-variable-in-esm]
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     snippets/ensogl-text-msdf-a35c812d6b3d67d9/msdfgen_wasm.js:1:940548:
 INFO ide_ci::program::command: npx⚠️       1 │ ...f(typeof exports==="object"){exports.ccall=ccall;exports.getValu...
 INFO ide_ci::program::command: npx⚠️         ╵                                 ~~~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️   This file is considered to be an ECMAScript module because of the "export" keyword here:
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️     snippets/ensogl-text-msdf-a35c812d6b3d67d9/msdfgen_wasm.js:2:2:
 INFO ide_ci::program::command: npx⚠️       2 │ ; export { ccall, getValue, _msdfgen_getKerning, _msdfgen_setVariat...
 INFO ide_ci::program::command: npx⚠️         ╵   ~~~~~~
 INFO ide_ci::program::command: npx⚠️
 INFO ide_ci::program::command: npx⚠️ 2 warnings and 1 error

Damn, I thought it is fixed ... we had discussion about it and recently it stopped showing up :( Ehh, so bad it's still there :(

wdanilo · 2023-05-04T18:05:46Z

app/ide-desktop/lib/types/modules.d.ts

+declare module '*/ensogl-pack/linked-dist' {
+    // eslint-disable-next-line no-restricted-syntax
+    export * from '../../../../lib/rust/ensogl/pack/js/src/runner/index'
+}


What does it do and why it is better than the previous version? I'm asking just to understand what is the difference here.

It was working fine earlier without that change. Did it stop working because of some new lints enabled? What are these lints? I'm asking about it as it makes the code extremely verbose here and I'm not sure it gives any benefits to us.

i have no clue whether this is a proper fix - but the idea is that this removes the dependency on the built module, so we are now pointing at the actual source (which always exists) instead of the built .d.ts

i don't think so - as mentioned elsewhere i think it only happens on clean runners (?!). the issue happens on the typecheck run, so what's happening is, typescript is looking for the type definitions but cannot find them

wdanilo · 2023-05-04T18:36:57Z

Oh, I understand this is fixing the CI errors we have now? If so, few questions:

Why did we merge a PR to develop which is breaking the CI? Was the CI working on the merged PR? How did it happen?
You've written These errors do not show up locally as the imports point to built assets. - but the CI is doing exactly the same things what you do locally, it is just running the ./run script, so how the results could be different?

wdanilo · 2023-05-04T18:38:44Z

As this fixes CI, Im merging it, but let's continue the discussion about the points above here please @somebody1234 .

somebody1234 · 2023-05-05T03:02:55Z

why did we merge a PR that broke the CI?

afaict, CI was working. still not sure why it breaks sometimes

the CI is doing exactly the same things what you do locally

i'm guessing the issue only appears on a fresh runner, which doesn't have build.json and ensogl-pack/linked-dist, so when TS tries to find the files, it can't find anything

wdanilo · 2023-05-05T03:46:13Z

why did we merge a PR that broke the CI?

afaict, CI was working. still not sure why it breaks sometimes

the CI is doing exactly the same things what you do locally

i'm guessing the issue only appears on a fresh runner, which doesn't have build.json and ensogl-pack/linked-dist, so when TS tries to find the files, it can't find anything

Ah, gotcha! Ok, that makes sense. Would you be so nice and also take a look at my code comment above? :)

This reverts commit 0a8f809.

* Revert "Fix lint CI (#6567)" This reverts commit 0a8f809. * Revert "Run typecheck and eslint on `./run lint` (#6314)" This reverts commit 7885145.

Fix lint CI

27bbfe1

somebody1234 added the CI: No changelog needed Do not require a changelog entry for this PR. label May 4, 2023

somebody1234 marked this pull request as ready for review May 4, 2023 17:26

somebody1234 requested review from PabloBuchu, indiv0, MichaelMauderer and wdanilo as code owners May 4, 2023 17:26

wdanilo reviewed May 4, 2023

View reviewed changes

wdanilo merged commit 0a8f809 into develop May 4, 2023

wdanilo deleted the wip/sb/fix-lint-ci branch May 4, 2023 18:39

somebody1234 added a commit that referenced this pull request May 8, 2023

Revert "Fix lint CI (#6567)"

0a18a9f

This reverts commit 0a8f809.

mwu-tow pushed a commit that referenced this pull request May 8, 2023

Revert typescript CI Lint changes (#6602)

0540430

* Revert "Fix lint CI (#6567)" This reverts commit 0a8f809. * Revert "Run typecheck and eslint on `./run lint` (#6314)" This reverts commit 7885145.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TypeScript lint errors #6567

Fix TypeScript lint errors #6567

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023 •

edited

Loading

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023

wdanilo commented May 4, 2023

wdanilo May 4, 2023 •

edited

Loading

somebody1234 May 5, 2023

wdanilo commented May 4, 2023

wdanilo commented May 4, 2023

somebody1234 commented May 5, 2023

wdanilo commented May 5, 2023

Fix TypeScript lint errors #6567

Fix TypeScript lint errors #6567

Conversation

somebody1234 commented May 4, 2023

Pull Request Description

Important Notes

Checklist

somebody1234 commented May 4, 2023 • edited Loading

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023

somebody1234 commented May 4, 2023

wdanilo commented May 4, 2023

wdanilo May 4, 2023 • edited Loading

Choose a reason for hiding this comment

somebody1234 May 5, 2023

Choose a reason for hiding this comment

wdanilo commented May 4, 2023

wdanilo commented May 4, 2023

somebody1234 commented May 5, 2023

wdanilo commented May 5, 2023

somebody1234 commented May 4, 2023 •

edited

Loading

wdanilo May 4, 2023 •

edited

Loading