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

Remove lodash #554

Merged
merged 3 commits into from Mar 24, 2023
Merged

Remove lodash #554

merged 3 commits into from Mar 24, 2023

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Oct 26, 2021

Requires #548 and #552.

Fixes #399.

Not sure if we export the validateTypes functions somewhere else or if they are even part of Stylelint's public API.

@kristerkari
Copy link
Collaborator

@XhmikosR From what I have understood previously, is that using any non-public methods from the stylelint library is risky because they might break/change at any time in the future.

@XhmikosR
Copy link
Member Author

XhmikosR commented Oct 26, 2021

Yeah, hence this might be a good chance to expose these utils upstream @jeddy3.

Otherwise, I will copy paste the source and tests from upstream, but it make sense to have these available from upstream. Or from a common util package maybe.

@jeddy3
Copy link
Collaborator

jeddy3 commented Oct 26, 2021

Yeah, hence this might be a good chance to expose these utils upstream @jeddy3.

Yes, we can add more utils to the public API. These validateOptions-related functions are a good candidate, as are the isStandardSyntax* utils.

Can you open an issue upstream so the rest of the team can weight-in, please?

(It's likely we'll use a bundler for Stylelint at some point, and these non-public methods won't be available.)

@XhmikosR
Copy link
Member Author

@jeddy3 done stylelint/stylelint#5669!

@XhmikosR
Copy link
Member Author

Looking at stylelint-scss code, it seems we duplicate a lot more utils from stylelint. Like most of the utils we have here are from upstream. We should try to see if it's worth exposing those upstream too.

@XhmikosR XhmikosR force-pushed the rm-more-lodash branch 3 times, most recently from 3525bb1 to a1b6e12 Compare November 1, 2021 12:45
@dzienisz
Copy link

dzienisz commented Aug 4, 2022

can we merge that? lodash is a vulnerabilities hell

@XhmikosR XhmikosR marked this pull request as ready for review March 23, 2023 15:15
@XhmikosR
Copy link
Member Author

@kristerkari LGTY? Do you have any suggestions? Would be nice to land this in minor/patch release.

Copy link
Collaborator

@kristerkari kristerkari left a comment

Choose a reason for hiding this comment

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

The changes look good, so unless there are some breaking changes we can just release this in the next minor or patch release. 👍

@XhmikosR XhmikosR merged commit e4b4508 into master Mar 24, 2023
16 checks passed
@XhmikosR XhmikosR deleted the rm-more-lodash branch March 24, 2023 12:43
@XhmikosR
Copy link
Member Author

@kristerkari can you cut a minor release please with the fixes so far please ❤️?

I don't have npm publish rights myself (BTW should enable 2FA or automation token in npm package settings)

@kristerkari
Copy link
Collaborator

@XhmikosR sure, but could you prepare some kind of changelog for a minor release? I can also give you the publish rights if you tell me your npm username (or is it the same one as your github handle?).

@XhmikosR
Copy link
Member Author

XhmikosR commented Mar 26, 2023

My npm handle is the same as here xhmikosr, thanks!

As for the release notes, GitHub now offers auto-generation. Personally, I'd just deprecate CHANGELOG.md and point to the releases section for future releases.

## What's Changed

* build(deps-dev): bump stylelint from 15.2.0 to 15.3.0 by @dependabot in https://github.com/stylelint-scss/stylelint-scss/pull/782
* build(deps-dev): bump prettier from 2.8.4 to 2.8.5 by @dependabot in https://github.com/stylelint-scss/stylelint-scss/pull/783
* build(deps-dev): bump @babel/core from 7.21.0 to 7.21.3 by @dependabot in https://github.com/stylelint-scss/stylelint-scss/pull/784
* build(deps-dev): bump np from 7.6.3 to 7.6.4 by @dependabot in https://github.com/stylelint-scss/stylelint-scss/pull/785
* Update CI by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/788
* Add .npmrc for older npm compatibility by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/789
* Update minor/patch dependencies and regenerate package-lock.json by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/790
* Minor docs fixes by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/791
* docs: fix broken links by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/792
* Remove lodash by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/554
* build(deps-dev): bump prettier from 2.8.6 to 2.8.7 by @dependabot in https://github.com/stylelint-scss/stylelint-scss/pull/793
* Move things outside of package.json by @XhmikosR in https://github.com/stylelint-scss/stylelint-scss/pull/794


**Full Changelog**: https://github.com/stylelint-scss/stylelint-scss/compare/v4.5.0...v4.6.0

@kristerkari
Copy link
Collaborator

@XhmikosR I don't personally like the auto-generated changelogs. The reason is that IMO the changelogs should describe the changes from the library's user's perspective, not from the library's developer's perspective (e.g. the user probably does not care if we have updated the CI config).

The one that you just posted is a good example of it being really noisy and making it difficult to tell if something meaningful has changed in the library, or if the changes are just some small internal tweaks.

@XhmikosR
Copy link
Member Author

XhmikosR commented Mar 26, 2023

Yeah, but I prefer to be able to move fast than have such things to worry about...

Plus, you can tweak it as you want, it's just works as a basis.

EDIT:

## What's Changed

* Docs: fix broken links and wording tweaks
* Remove lodash

**Full Changelog**: https://github.com/stylelint-scss/stylelint-scss/compare/v4.5.0...v4.6.0

@kristerkari
Copy link
Collaborator

@XhmikosR sorry, but I really don't share the same opinion in this case.

Saving a few minutes from making a changelog when creating a new release is not a priority for me. I have been maintaining this library for the last 7 years, and you can go to the release page and see what exactly has changed from the user's perspective in every release: https://github.com/stylelint-scss/stylelint-scss/releases

The new changelog that you posted is much better as it describes the meaningful changes and doc updates that affect the user. 👍

@XhmikosR
Copy link
Member Author

XhmikosR commented Mar 26, 2023

We can agree to disagree :)

I'm not saying you are wrong, I prefer your approach too. I just don't want to spend a lot of time on the CHANGELOG.md.

Regardless, IMHO it should be deprecated and just point to the GitHub Releases since it's one less thing to handle when cutting a release. But it's your call of course :)

@kristerkari
Copy link
Collaborator

@XhmikosR if you don't want to spend time on it, just let me know when you need it and I'll make a new release and update the changelog. I don't mind doing it manually if the end result is clean and readable.

@XhmikosR
Copy link
Member Author

Sounds good, thanks!

Let's cut a 4.6.0 release for now and then I have #795 and the rm-babel branch which I want to hear your feedback there before I proceed with redoing the changes.

@kristerkari
Copy link
Collaborator

I think that #795 should be fine, and removing Babel needs a major realse, right?

@XhmikosR
Copy link
Member Author

XhmikosR commented Mar 26, 2023

Woo-hoo!

https://packagephobia.com/result?p=stylelint-scss

image

re: babel: yeah.

As for #795 let's discuss that there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Reduce lodash usage
4 participants