yanyiwu / nodejieba

"结巴"中文分词的Node.js版本
MIT License
3.01k stars 278 forks source link

chore: fix publish secret and use correct runner os #231

Closed yeliex closed 8 months ago

yeliex commented 9 months ago

this pr fix invalid configuration of github ci test and release, including run runnings in correctly os and use github default secret token.

it should build pre-build binaries as expected and upload them to github node registry without specify token manually.

yeliex commented 9 months ago

@yanyiwu @Mister-Hope review please

as far as I am concerned, it is really heavy for a such simply node repo to use pnpm(it cause cannot support node<16) and rollup(it just has a ts file only, tsc or swc is enough)

Mister-Hope commented 9 months ago

The latest change is on main. Change your target to that.

The latest code on master never get published.

I have emailed the original author but not getting feedback.

I have no authority with settings panel and I do not have access to the pkg.

If you want, you can use nodejs-jieba, that's a pkg I am maintening.

yeliex commented 9 months ago

@Mister-Hope as my pr described, use secret.GITHUB_TOKEN instead of your custom secret

feel free to update or publish if u has permission

Mister-Hope commented 9 months ago

The basic problem is that I even do not have access to publish package to npm.....

Mister-Hope commented 9 months ago

Change your target branch to main plz, node 16 is no longer supported anymore, and we have some updates on main branch about the project.

Mister-Hope commented 9 months ago

If you are sure that GITHUB_TOKEN is enough for the action used in workflow, an update would be appreciated (though we still can not publish any package)

Edited: I believe you may also need to update permission field.

yeliex commented 9 months ago

it is long history project, many users in big companies want to use in even node@12 (include me some times), as it first supports. In my opinion if could support without more efficient, we should do it. it looks like unnecessary to do much change, keep currently and support new node napi pre-build binaries is enough

let @yanyiwu decide which version should be supported, he told me would pay attention when free on weekends

Mister-Hope commented 9 months ago

Every project should have a clear env requirement. If a project requires to be running on a non-LTS version of node.js, then they should stay on v2. Package like this with 0 reps won't met security issues from upstream.

I even want v3 to be a pure esm package. Dropping old codes and moving to modern is exactly a major version should do.

yeliex commented 9 months ago

new major update without any new features is unnecessary.

besides a reason why cannot use another package name instead: big companies limited connections to official npm registry or github, keep original name would help them continue use npmmirror to get prebuilt binaries

Mister-Hope commented 9 months ago

Changes with cpp could be sync to v2 branch and ships with new v2 version, no problem for that. So no actual problem for a new major to target LTS node.js

Mister-Hope commented 9 months ago

Why a pure esm package targeting lts with dts shipped is not "a necessary refactor" suitable for major?

Also a lot of reps has major version bumped.

Let's make it clear, a major bump for "node-gyp" is already worthy enough for v3

yeliex commented 9 months ago

for declare: drop node16 support is unnecessary

it should keep supporting, it is required to ship pure esm package for major release, not related to node 16

if want a pure esm major version, why not support any version supports esm if no other advantages.

Currently v2 has no prebuilt binaries for node16. if could, add support to this and bump a v2 patch version please

Thats why i create this pr to master not main

Mister-Hope commented 9 months ago

Node16 is already eol, isn't it? There is no disadvantage for you to stay on v2 to use node16.

Changes with cpp could be sync to v2 branch and ships with new v2 version, no problem for that. So no actual problem for a new major to target LTS node.js

yanyiwu commented 9 months ago

This branch has conflicts that must be resolved Use the web editor or the to resolve conflicts. Conflicting files .github/workflows/release.yml .github/workflows/test.yml