8000 Fix many ESlint errors by hoodmane · Pull Request #1265 · pyscript/pyscript · GitHub
[go: up one dir, main page]

Skip to content

Fix many ESlint errors #1265

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

Merged
merged 30 commits into from
Mar 13, 2023
Merged

Fix many ESlint errors #1265

merged 30 commits into from
Mar 13, 2023

Conversation

hoodmane
Copy link
Contributor
@hoodmane hoodmane commented Mar 8, 2023

For mysterious reasons, these errors appear on my branch #1262 even
though they are not related to changes there. The eslint config seems
a bit unstable.

Anyways this fixes them.

hoodmane added 3 commits March 8, 2023 12:30
For mysterious reasons, these errors appear on my branch pyscript#1262 even
though they are not related to changes there. The eslint config seems
a bit unstable.

Anyways this fixes them.
@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 8, 2023

I'm really confused about the behavior of eslint. Locally I see quite different output...

@JeffersGlass
Copy link
Contributor

I wouldn't swear it's the same issue, but we had a weird thing happen with ESLint back in the fall, where a particular objectionable line was 'warning' locally (when running in desktop Ubuntu 22.04) but 'erroring' in CI (on GitHub's docker image of Ubuntu 22.04). Didn't ever get to the bottom of exactly what the difference was, but running that docker image locally allowed for more visibility into the differences.

@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 8, 2023

Thanks for the info @JeffersGlass, is the github actions ubuntu 22.04 image available on docker hub? I'm having trouble finding it.

@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 8, 2023

Though I guess that's a useless direction because we are running it on pre-commit.ci and not on github actions.

@tedpatrick
Copy link
Contributor

👏 👏 👏 👏 👏

@tedpatrick
Copy link
Contributor

Still using toml-j0.4?

@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 9, 2023

Still using toml-j0.4?

Yes. I'm not sure what the previous investigation into toml libraries was, but currently as far as I can tell there are four main options:

  • toml -- not maintained for four years, toml 0.4, 860k weekly downloads, 110kb bundle
  • toml-j0.4 -- not maintained for six years, toml 0.4, 16k weekly downloads, 22kb bundle
  • @ltd/j-toml -- actively maintained, toml 1.0, 40k weekly downloads, 62kb bundle
  • @iarna/toml -- actively maintained, toml 0.5, 2700k weekly downloads, 50kb bundle, primarily targets node, browser is an afterthought (no support for no-unsafe-eval)

There are a bunch of other options with <1k weekly downloads -- some of these look promising but it's better to stick with popular libs. I think @ltd/j-toml is the best for us followed by toml-j0.4. If we decide to switch to @ltd/j-toml we can do it in a followup.

@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 9, 2023

Could someone merge this? I think it's ready.

@tedpatrick
Copy link
Contributor
tedpatrick commented Mar 9, 2023 via email

@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 9, 2023

fast-toml also has alarmingly few users compared to the other options listed...

@hoodmane
Copy link
Contributor Author
hoodmane commented Mar 9, 2023

One problem with toml-j0.4 is that it has type definitions in toml.d.ts but it fails to include them in the files list in package.json so they are not actually usable. The type definitions file is very short so we could also just vendor it.

On the other hand, @ltd/j-toml and toml both correctly bundle type definitions.

Comment on lines 40 to 42
# toml-j0.4 left out its types from the npm dist
# See https://github.com/jakwings/toml-j0.4/pull/8
cd node_modules/toml-j0.4 && wget https://raw.githubusercontent.com/jakwings/toml-j0.4/v1.1.1/toml.d.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedpatrick workaround for toml-j0.4's missing types

@@ -49,7 +49,7 @@
"@codemirror/state": "6.1.2",
"@codemirror/theme-one-dark": "6.1.0",
"@codemirror/view": "6.3.0",
"codemirror": "6.0.1",
"toml-j0.4": "^1.1.1"
"@hoodmane/toml-j0.4": "^1.1.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vyings merged my pull request adding the type definitions to the toml-j0.4 distribution, and tagged a new version v1.1.2. However he says he has lost access to the npm registry account so I published v1.1.2 of tom-j0.4 here. This way we don't need to wget to patch up the types.

@antocuni antocuni merged commit 37c9db0 into pyscript:main Mar 13, 2023
@hoodmane hoodmane deleted the fix-lints branch March 13, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0