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.

Sign up for GitHub

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

Jump to bottom

Move stylelint to 14 #242

Merged
merged 4 commits into from
Nov 29, 2021
Merged

Move stylelint to 14 #242

merged 4 commits into from
Nov 29, 2021

Conversation

skjnldsv
Copy link
Contributor

Signed-off-by: John Molakvoæ skjnldsv@protonmail.com

@skjnldsv skjnldsv added the dependencies Pull requests that update a dependency file label Nov 11, 2021
@juliusknorr

This comment has been minimized.

Sign in to view
@juliusknorr juliusknorr mentioned this pull request Nov 11, 2021
Merged
@skjnldsv
Copy link
Contributor Author

Should be good, please review

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Nov 11, 2021 •
edited
Loading

ERROR in Could not find "stylelint-config-recommended-vue". Do you need a configBasedir?

Oups

EDIT: nextcloud-libraries/stylelint-config#16

@skjnldsv skjnldsv mentioned this pull request Nov 15, 2021
Merged
@artonge artonge mentioned this pull request Nov 22, 2021
Merged
@artonge artonge force-pushed the feat/stylelint-14 branch 2 times, most recently from 9ff3c77 to fe33edc Compare November 22, 2021 14:44
@artonge
Copy link
Contributor

artonge commented Nov 22, 2021 •
edited
Loading

Added postcss-html to dependencies to make CI pass. Not sure if that is the best approach.

This was referenced Nov 22, 2021
Closed
Closed
@juliusknorr
Copy link
Contributor

Right, i think it would make sense to rather add that to https://github.com/nextcloud/stylelint-config then, seems rather to be a peer dependency of that config

@artonge
Copy link
Contributor

artonge commented Nov 22, 2021 •
edited
Loading

Right, i think it would make sense to rather add that to https://github.com/nextcloud/stylelint-config then, seems rather to be a peer dependency of that config

The thing is, that it is this repo that needs it. @nextcloud/stylelint-config already has it as a dependency:

$ npm why postcss-html
> npm ERR! No dependencies found matching postcss-html

$ cd tests/

$ npm why postcss-html
> postcss-html@1.2.0 dev peer
node_modules/postcss-html
  peer postcss-html@"^1.0.0" from stylelint-config-html@1.0.0
  node_modules/stylelint-config-html
    stylelint-config-html@">=1.0.0" from stylelint-config-recommended-vue@1.0.0
    node_modules/stylelint-config-recommended-vue
      peer stylelint-config-recommended-vue@"^1.0.0" from @nextcloud/stylelint-config@2.0.1
      node_modules/@nextcloud/stylelint-config
        dev @nextcloud/stylelint-config@"^2.0.1" from the root project
  peer postcss-html@"^1.0.0" from stylelint-config-recommended-vue@1.0.0
  node_modules/stylelint-config-recommended-vue
    peer stylelint-config-recommended-vue@"^1.0.0" from @nextcloud/stylelint-config@2.0.1
    node_modules/@nextcloud/stylelint-config
      dev @nextcloud/stylelint-config@"^2.0.1" from the root project

In a normal scenario, I think that the tests would pass, but the fact the we use "@nextcloud/webpack-vue-config": "file:.." here, "splits" the node_modules folder in two, one in the root (/) folder and one in tests, which lead to webpack not finding the postcss-html package.

/ (@nextcloud/webpack-vue-config)
| - node_modules
    | - webpack
    | - ...
| - tests
    | - node_modules
        | - @nextcloud/stylelint-config
        | - postcss-html
        | - ...

Normal scenario would be:

/
| - node_modules
    | - webpack
    | - @nextcloud/stylelint-config
    | - postcss-html
    | - ...

@artonge
Copy link
Contributor

artonge commented Nov 22, 2021

Attempting to use npm pack

@artonge artonge force-pushed the feat/stylelint-14 branch 9 times, most recently from 09ddcab to 15a30c6 Compare November 22, 2021 17:15
@artonge
Copy link
Contributor

artonge commented Nov 22, 2021

CI is green when using npm pack and installing the resulting archive.

@st3iny st3iny mentioned this pull request Nov 23, 2021
Merged
@artonge
Copy link
Contributor

artonge commented Nov 29, 2021

@skjnldsv An opinion on the new npm-test.yml workflow?

@skjnldsv
Copy link
Contributor Author

@skjnldsv An opinion on the new npm-test.yml workflow?

Yeah, looks nice!
I figured "@nextcloud/webpack-vue-config": "file:.." looked broken! 😕

Your change could use some comments to ensure anyone understand why we're doing this :)

skjnldsv and others added 3 commits November 29, 2021 12:21
@skjnldsv
Move stylelint to 14
dfe55a2 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
Use latest stylelint-config for stylelint14
16eacff 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@artonge @skjnldsv
Use npm pack in npm-test workflow
e9e4133 
Signed-off-by: Louis Chemineau <louis@chmn.me>
@skjnldsv
Copy link
Contributor Author

Tests looks good @artonge, wanna merge now?

@artonge artonge merged commit 1fdf4b8 into master Nov 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/stylelint-14 branch November 29, 2021 12:42
@skjnldsv
Copy link
Contributor Author

Release?

@artonge
Copy link
Contributor

artonge commented Nov 29, 2021

Major or minor ?
I would say minor, nothing is breaking change.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Nov 29, 2021 •
edited
Loading

Minor yep :)
But I assume it will break if it's not updated alongside the config anyway, so should be safe for the auto-merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skjnldsv @juliusknorr @artonge