Skip to content
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

Use shared eslint config #2564

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

adrianschmidt
Copy link
Contributor

Review:

  • Commits are atomic
  • Commits have the correct type for the changes made
  • Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

  • Chrome
  • Edge
  • Firefox

Linux:

  • Chrome
  • Firefox

macOS:

  • Chrome
  • Firefox
  • Safari

Mobile:

  • Chrome on Android
  • iOS

@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch from 3a97fce to 3d1bfb7 Compare October 29, 2023 09:11
@github-actions
Copy link

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-2564/

@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch from 3d1bfb7 to 4f85ae3 Compare October 29, 2023 09:56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this work on Windows (using a symlink that is)? Can you help me out by checking @mjlime?

Just pull this branch down, run npm i and npm run lint. If it doesn't work, you should be getting either an error about there being something wrong with your .prettierrc, or, as I got when I simply removed the symlink, 7,674 lint errors 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested, and it works when running on Windows in GitHub actions at least 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like it works:

...
Oops! Something went wrong! :(

ESLint: 8.52.0

Error: Cannot find package 'node_modules' imported from C:\limecrm\Source\lime-elements\.prettierrc
    at new NodeError (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:18797:5)
    at packageResolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19742:9)
    at moduleResolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19774:20)
    at defaultResolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19879:15)
    at resolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19897:12)
    at importFromFile (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19912:15)
    at loadExternalConfig (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19931:24)
    at Object.transform (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19979:20)
    at run (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/internal/internal.mjs:5757:36)
    at async cacheWrapper (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/internal/internal.mjs:55:22)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm… that's a weird error… it seems the symlink worked, because it complains about not finding modules referenced from the symlinked file.

Which npm version are you running?

Copy link
Contributor

Choose a reason for hiding this comment

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

└$ npm --version
9.8.1
└$ node --version
v18.18.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.

Ah crap… I was hoping you were using npm <7, because then the problem would have simply been that peer dependencies didn't get installed automatically.

But since that isn't the case, I don't know what the problem is, and that makes me sad… 😢

I'll look into if we can maybe copy the .prettierrc from @limetech/eslint-config to the consumer package in the postinstall… 🤔

Copy link
Contributor Author

@adrianschmidt adrianschmidt Oct 31, 2023

Choose a reason for hiding this comment

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

@mjlime Ok, now it ought to work. I've put the normal .prettierrc back, and I also put a post-install script in Lundalogik/eslint-config that copies its .prettierrc to the consumer project, but only if there isn't already a .prettierrc file there. So it won't overwrite anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adrianschmidt Still running into problems (npm 9.8.1, node 18.18.2):

$ npm i;npm run lint

up to date, audited 1071 packages in 12s

299 packages are looking for funding
  run `npm fund` for details

8 vulnerabilities (3 moderate, 5 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

> @limetech/lime-elements@37.1.0-next.49 lint
> npm run lint:src && npm run lint:scss


> @limetech/lime-elements@37.1.0-next.49 lint:src
> eslint "**/*.{ts,tsx,js}" --max-warnings=0


Oops! Something went wrong! :(

ESLint: 8.52.0

Error: Cannot find package 'node_modules' imported from C:\limecrm\Source\lime-elements\.prettierrc
    at new NodeError (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:18797:5)
    at packageResolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19742:9)
    at moduleResolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19774:20)
    at defaultResolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19879:15)
    at resolve (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19897:12)
    at importFromFile (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19912:15)
    at loadExternalConfig (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19931:24)
    at Object.transform (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/index.mjs:19979:20)
    at run (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/internal/internal.mjs:5757:36)
    at async cacheWrapper (file:///C:/limecrm/Source/lime-elements/node_modules/prettier/internal/internal.mjs:55:22)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, removing .prettierrc and running it again gave me this:

✖ 46839 problems (46839 errors, 0 warnings)
  46839 errors and 0 warnings potentially fixable with the `--fix` option.

Improvement? You tell me @adrianschmidt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to need to get myself a Windows VM if I'm ever to get this to work, I think… 🤔

@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch 2 times, most recently from 2148753 to b1a4c67 Compare October 30, 2023 09:05
@adrianschmidt adrianschmidt self-assigned this Oct 30, 2023
@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch 2 times, most recently from 867a847 to b1a4c67 Compare October 30, 2023 09:22
@adrianschmidt adrianschmidt marked this pull request as ready for review October 30, 2023 09:23
@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch 5 times, most recently from 7363d6c to a4dffe0 Compare October 31, 2023 16:37
@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch from a4dffe0 to 82e9486 Compare November 7, 2023 14:39
@adrianschmidt adrianschmidt force-pushed the use-shared-eslint-config branch from 82e9486 to cbece01 Compare November 8, 2023 19:31
@adrianschmidt adrianschmidt marked this pull request as draft November 13, 2023 08:18
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.

2 participants