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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

yeliex
Copy link
Contributor

@yeliex yeliex commented Dec 14, 2023

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
Copy link
Contributor Author

yeliex commented Dec 14, 2023

@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
Copy link
Collaborator

Mister-Hope commented Dec 14, 2023

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
Copy link
Contributor Author

yeliex commented Dec 14, 2023

@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
Copy link
Collaborator

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

@Mister-Hope
Copy link
Collaborator

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 Mister-Hope changed the base branch from master to main December 14, 2023 13:27
@Mister-Hope
Copy link
Collaborator

Mister-Hope commented Dec 14, 2023

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
Copy link
Contributor Author

yeliex commented Dec 14, 2023

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
Copy link
Collaborator

Mister-Hope commented Dec 14, 2023

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
Copy link
Contributor Author

yeliex commented Dec 14, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

Mister-Hope commented Dec 14, 2023

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
Copy link
Contributor Author

yeliex commented Dec 14, 2023

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
Copy link
Collaborator

Mister-Hope commented Dec 14, 2023

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
Copy link
Owner

yanyiwu commented Dec 16, 2023

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

@yeliex yeliex closed this Jan 12, 2024
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.

None yet

3 participants