-
-
Notifications
You must be signed in to change notification settings - Fork 508
chore: improve package.json #695
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
base: main
Are you sure you want to change the base?
Conversation
7d57444 to
7b9a7ab
Compare
| @@ -1,13 +1,12 @@ | |||
| import cls from "classnames"; | |||
| import s from "./Button.css"; | |||
| import * as s from "./Button.css"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default all classnames in named export (for better tree shaking)
| const classes = cls(className, { | ||
| [s.button]: true, | ||
| [s.active]: active, | ||
| [s.toggle]: toggle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have toggle class
| onFocus={this.handleFocus} | ||
| /> | ||
| {this.state.showOptions ? ( | ||
| <div className={s.options}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have options class in CSS
| import escapeRegExp from "escape-string-regexp"; | ||
| import { escape } from "html-escaper"; | ||
| import filesize from "filesize"; | ||
| import { filesize } from "filesize"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we need to use named export
| highlightedModules: computed, | ||
| foundModulesInfo: computed | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using decorators (they are deprecated in mobx too)
|
|
||
| :export { | ||
| toggleTime: 200ms; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using this in JS, so let's export this value using ICSS :export
| hasConcatenatedModules: computed, | ||
| foundModulesSize: computed, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using decorators (they are deprecated in mobx too)
| const { resolve, dirname } = require("path"); | ||
|
|
||
| const commander = require("commander"); | ||
| const { program: commanderProgram } = require("commander"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commander now uses named export and ask to provide argument using argument()
| @@ -1,6 +1,5 @@ | |||
| const fs = require("fs"); | |||
| const path = require("path"); | |||
| const del = require("del"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use del, NodeJS has good fs module
| "devDependencies": { | ||
| "@babel/cli": "^7.28.6", | ||
| "@babel/core": "7.26.9", | ||
| "@babel/plugin-proposal-decorators": "7.25.9", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need @babel/plugin-proposal-decorators anymore
| react: "preact/compat", | ||
| "react-dom/test-utils": "preact/test-utils", | ||
| "react-dom": "preact/compat", | ||
| mobx: require.resolve("mobx/lib/mobx.es6.js"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need, mobx by default provide es6 version
|
@valscion Ready for review, updated all deps |
valscion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all looks good to me! Is there perhaps one stray console.log left over? Let's remove it if it was unnecessary and then it's OK to merge.
src/bin/analyzer.js
Outdated
| .parse(process.argv); | ||
| .parse(); | ||
|
|
||
| console.log(program.args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this console.log here on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valscion My typo, forget when tests because we don't have tests for such things, removed, in future we will have no-console in eslint-config-webpack and will catch such things
|
@valscion ready to review again |
Summary
Here:
packageManagerversion to be align withengines.node, v20.9.0 include by defaultnpm@10.1.0^rangeWhat kind of change does this PR introduce?
chore
Did you add tests for your changes?
Existing
Does this PR introduce a breaking change?
No
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing