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 esbuild from devDeps but keep testing in CI, fix #139 #149

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

fritx
Copy link
Collaborator

@fritx fritx commented Jan 3, 2024

fix #139 (comment)

Below is output from [[ git diff 16334ea 4e0cb43 -- bun.lockb ]], ref: https://bun.sh/docs/install/lockfile

I can't prevent the unrelated version change (marked@^9.1.6), if you guys know how to improve pls tell me ;)

diff --git a/bun.lockb b/bun.lockb
index 3fce62c..38f7e48 100755
--- a/bun.lockb
+++ b/bun.lockb
@@ -1,123 +1,8 @@
 # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
 # yarn lockfile v1
-# bun ./bun.lockb --hash: 2CEF5A6DE7AD60F9-17c3d339555f4a51-36D751B931BC340D-779e4c6050698b75
+# bun ./bun.lockb --hash: DEEABC817505CDA6-f41a17c91aabae47-E5034553CF1C77BA-d5d19ed64226cf44
 
 
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/aix-ppc64/-/aix-ppc64-0.19.11.tgz"
-  integrity sha512-FnzU0LyE3ySQk7UntJO4+qIiQgI7KoODnZg5xzXIrFJlKd2P2gwHsHY4927xj9y5PJmJSzULiUCWmv7iWnNa7g==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/android-arm/-/android-arm-0.19.11.tgz"
-  integrity sha512-5OVapq0ClabvKvQ58Bws8+wkLCV+Rxg7tUVbo9xu034Nm536QTII4YzhaFriQ7rMrorfnFKUsArD2lqKbFY4vw==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/android-arm64/-/android-arm64-0.19.11.tgz"
-  integrity sha512-aiu7K/5JnLj//KOnOfEZ0D90obUkRzDMyqd/wNAUQ34m4YUPVhRZpnqKV9uqDGxT7cToSDnIHsGooyIczu9T+Q==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/android-x64/-/android-x64-0.19.11.tgz"
-  integrity sha512-eccxjlfGw43WYoY9QgB82SgGgDbibcqyDTlk3l3C0jOVHKxrjdc9CTwDUQd0vkvYg5um0OH+GpxYvp39r+IPOg==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/darwin-arm64/-/darwin-arm64-0.19.11.tgz"
-  integrity sha512-ETp87DRWuSt9KdDVkqSoKoLFHYTrkyz2+65fj9nfXsaV3bMhTCjtQfw3y+um88vGRKRiF7erPrh/ZuIdLUIVxQ==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/darwin-x64/-/darwin-x64-0.19.11.tgz"
-  integrity sha512-fkFUiS6IUK9WYUO/+22omwetaSNl5/A8giXvQlcinLIjVkxwTLSktbF5f/kJMftM2MJp9+fXqZ5ezS7+SALp4g==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/freebsd-arm64/-/freebsd-arm64-0.19.11.tgz"
-  integrity sha512-lhoSp5K6bxKRNdXUtHoNc5HhbXVCS8V0iZmDvyWvYq9S5WSfTIHU2UGjcGt7UeS6iEYp9eeymIl5mJBn0yiuxA==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/freebsd-x64/-/freebsd-x64-0.19.11.tgz"
-  integrity sha512-JkUqn44AffGXitVI6/AbQdoYAq0TEullFdqcMY/PCUZ36xJ9ZJRtQabzMA+Vi7r78+25ZIBosLTOKnUXBSi1Kw==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-arm/-/linux-arm-0.19.11.tgz"
-  integrity sha512-3CRkr9+vCV2XJbjwgzjPtO8T0SZUmRZla+UL1jw+XqHZPkPgZiyWvbDvl9rqAN8Zl7qJF0O/9ycMtjU67HN9/Q==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-arm64/-/linux-arm64-0.19.11.tgz"
-  integrity sha512-LneLg3ypEeveBSMuoa0kwMpCGmpu8XQUh+mL8XXwoYZ6Be2qBnVtcDI5azSvh7vioMDhoJFZzp9GWp9IWpYoUg==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-ia32/-/linux-ia32-0.19.11.tgz"
-  integrity sha512-caHy++CsD8Bgq2V5CodbJjFPEiDPq8JJmBdeyZ8GWVQMjRD0sU548nNdwPNvKjVpamYYVL40AORekgfIubwHoA==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-loong64/-/linux-loong64-0.19.11.tgz"
-  integrity sha512-ppZSSLVpPrwHccvC6nQVZaSHlFsvCQyjnvirnVjbKSHuE5N24Yl8F3UwYUUR1UEPaFObGD2tSvVKbvR+uT1Nrg==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-mips64el/-/linux-mips64el-0.19.11.tgz"
-  integrity sha512-B5x9j0OgjG+v1dF2DkH34lr+7Gmv0kzX6/V0afF41FkPMMqaQ77pH7CrhWeR22aEeHKaeZVtZ6yFwlxOKPVFyg==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-ppc64/-/linux-ppc64-0.19.11.tgz"
-  integrity sha512-MHrZYLeCG8vXblMetWyttkdVRjQlQUb/oMgBNurVEnhj4YWOr4G5lmBfZjHYQHHN0g6yDmCAQRR8MUHldvvRDA==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-riscv64/-/linux-riscv64-0.19.11.tgz"
-  integrity sha512-f3DY++t94uVg141dozDu4CCUkYW+09rWtaWfnb3bqe4w5NqmZd6nPVBm+qbz7WaHZCoqXqHz5p6CM6qv3qnSSQ==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-s390x/-/linux-s390x-0.19.11.tgz"
-  integrity sha512-A5xdUoyWJHMMlcSMcPGVLzYzpcY8QP1RtYzX5/bS4dvjBGVxdhuiYyFwp7z74ocV7WDc0n1harxmpq2ePOjI0Q==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/linux-x64/-/linux-x64-0.19.11.tgz"
-  integrity sha512-grbyMlVCvJSfxFQUndw5mCtWs5LO1gUlwP4CDi4iJBbVpZcqLVT29FxgGuBJGSzyOxotFG4LoO5X+M1350zmPA==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/netbsd-x64/-/netbsd-x64-0.19.11.tgz"
-  integrity sha512-13jvrQZJc3P230OhU8xgwUnDeuC/9egsjTkXN49b3GcS5BKvJqZn86aGM8W9pd14Kd+u7HuFBMVtrNGhh6fHEQ==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/openbsd-x64/-/openbsd-x64-0.19.11.tgz"
-  integrity sha512-ysyOGZuTp6SNKPE11INDUeFVVQFrhcNDVUgSQVDzqsqX38DjhPEPATpid04LCoUr2WXhQTEZ8ct/EgJCUDpyNw==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/sunos-x64/-/sunos-x64-0.19.11.tgz"
-  integrity sha512-Hf+Sad9nVwvtxy4DXCZQqLpgmRTQqyFyhT3bZ4F2XlJCjxGmRFF0Shwn9rzhOYRB61w9VMXUkxlBy56dk9JJiQ==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/win32-arm64/-/win32-arm64-0.19.11.tgz"
-  integrity sha512-0P58Sbi0LctOMOQbpEOvOL44Ne0sqbS0XWHMvvrg6NE5jQ1xguCSSw9jQeUk2lfrXYsKDdOe6K+oZiwKPilYPQ==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/win32-ia32/-/win32-ia32-0.19.11.tgz"
-  integrity sha512-6YOrWS+sDJDmshdBIQU+Uoyh7pQKrdykdefC1avn76ss5c+RN6gut3LZA4E2cH5xUEp5/cA0+YxRaVtRAb0xBg==
-
-"@esbuild/[email protected]":
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/@esbuild/win32-x64/-/win32-x64-0.19.11.tgz"
-  integrity sha512-vfkhltrjCAb603XaFhqhAF4LGDi2M4OrCRrFusyQ+iTLQ/o60QQXxc9cZC/FFpihBI9N1Grn6SMKVJ4KP7Fuiw==
-
 argparse@^2.0.1:
   version "2.0.1"
   resolved "https://registry.npmjs.org/argparse/-/argparse-2.0.1.tgz"
@@ -163,35 +48,6 @@ entities@^4.2.0, entities@^4.5.0:
   resolved "https://registry.npmjs.org/entities/-/entities-4.5.0.tgz"
   integrity sha512-V0hjH4dGPh9Ao5p0MoRY6BVqtwCjhz6vI5LT8AJ55H+4g9/4vbHx1I54fS0XuclLhDHArPQCiMjDxjaL8fPxhw==
 
-esbuild@^0.19.11:
-  version "0.19.11"
-  resolved "https://registry.npmjs.org/esbuild/-/esbuild-0.19.11.tgz"
-  integrity sha512-HJ96Hev2hX/6i5cDVwcqiJBBtuo9+FeIJOtZ9W1kA5M6AMJRHUZlpYZ1/SbEwtO0ioNAW8rUooVpC/WehY2SfA==
-  optionalDependencies:
-    "@esbuild/aix-ppc64" "0.19.11"
-    "@esbuild/android-arm" "0.19.11"
-    "@esbuild/android-arm64" "0.19.11"
-    "@esbuild/android-x64" "0.19.11"
-    "@esbuild/darwin-arm64" "0.19.11"
-    "@esbuild/darwin-x64" "0.19.11"
-    "@esbuild/freebsd-arm64" "0.19.11"
-    "@esbuild/freebsd-x64" "0.19.11"
-    "@esbuild/linux-arm" "0.19.11"
-    "@esbuild/linux-arm64" "0.19.11"
-    "@esbuild/linux-ia32" "0.19.11"
-    "@esbuild/linux-loong64" "0.19.11"
-    "@esbuild/linux-mips64el" "0.19.11"
-    "@esbuild/linux-ppc64" "0.19.11"
-    "@esbuild/linux-riscv64" "0.19.11"
-    "@esbuild/linux-s390x" "0.19.11"
-    "@esbuild/linux-x64" "0.19.11"
-    "@esbuild/netbsd-x64" "0.19.11"
-    "@esbuild/openbsd-x64" "0.19.11"
-    "@esbuild/sunos-x64" "0.19.11"
-    "@esbuild/win32-arm64" "0.19.11"
-    "@esbuild/win32-ia32" "0.19.11"
-    "@esbuild/win32-x64" "0.19.11"
-
 htmlparser2@^9.0.0:
   version "9.0.0"
   resolved "https://registry.npmjs.org/htmlparser2/-/htmlparser2-9.0.0.tgz"
@@ -209,31 +65,13 @@ js-yaml@^4.1.0:
   dependencies:
     argparse "^2.0.1"
 
-marked@^9.1.2:
+marked@^9.1.6:
   version "9.1.6"
   resolved "https://registry.npmjs.org/marked/-/marked-9.1.6.tgz"
   integrity sha512-jcByLnIFkd5gSXZmjNvS1TlmRhCXZjIzHYlaGkPlLIekG55JDR2Z4va9tZwCiP+/RDERiNhMOFu01xd6O5ct1Q==
 
-nuejs-core@^0.3.0:
-  version "0.3.0"
-  resolved "https://registry.npmjs.org/nuejs-core/-/nuejs-core-0.3.0.tgz"
-  integrity sha512-8RTa3Hz+tE9dfeBe01+oT6kKhknMmETTPc3Ap1tURCmzNdvXamUFbt2FNaHi1OBmGWk0i8FYKFvhj6ZtCSpO9Q==
-  dependencies:
-    htmlparser2 "^9.0.0"
-
-"nuejs-core@packages/nuejs":
+"nuejs-core@nuejs-core":
   version "workspace:packages/nuejs"
   resolved "workspace:packages/nuejs"
   dependencies:
     htmlparser2 "^9.0.0"
-
-"nuekit@packages/nuekit":
-  version "workspace:packages/nuekit"
-  resolved "workspace:packages/nuekit"
-  devDependencies:
-    esbuild "^0.19.11"
-  dependencies:
-    diff-dom "^5.0.6"
-    js-yaml "^4.1.0"
-    marked "^9.1.2"
-    nuejs-core "^0.3.0"

@tipiirai tipiirai merged commit 4e0cb43 into nuejs:master Jan 3, 2024
1 check passed
@tipiirai
Copy link
Contributor

tipiirai commented Jan 3, 2024

Great job!

Btw: I just noticed the "Checks" tabs on this commit. I haven't used this kind of CI- flow in the past. Looks solid. Is .github/workflows/test.yaml enough to enable continuous integration?

Also, I'm not familiar with these pnpm/bun lock files. Can you explain them to me (like I was 10 years old) and what should I do about them?

fritx added a commit to fritx/nue that referenced this pull request Jan 3, 2024
tipiirai added a commit that referenced this pull request Jan 3, 2024
Revert unnecessary changes of bun.lockb in PR #149
@fritx
Copy link
Collaborator Author

fritx commented Jan 3, 2024

Btw: I just noticed the "Checks" tabs on this commit. I haven't used this kind of CI- flow in the past. Looks solid. Is .github/workflows/test.yaml enough to enable continuous integration?

The CI / CD can include many things, for example:

But according to nuejs CONTRIBUTING.md , I'm not sure which of them you want for it, for keeping its simplicity (I love simplicity too 😂 )

fritx added a commit to fritx/nue that referenced this pull request Jan 8, 2024
@goblinfactory
Copy link
Contributor

goblinfactory commented Feb 1, 2024

We might need to circle back and decide on a policy for bun.locb file. I've not used bun before but the fact that it's binary creates some obvious issues. For the short term perhaps just add it to .gitignore and at a later stage include a human friendly version as well, or ... discuss the options for doing so? So that if something breaks the builds due to transitive dependancy we're equipped to identify the specific package and manually add a specific frozen version to to package.json, if and when that ever happens.

I did see this plugin for vscode where you can preview the lock file changes;
https://marketplace.visualstudio.com/items?itemName=jaaxxx.bun-lockb
I've not used it, just mentioning it so that we can discuss later.

For now we should perhaps add 'bun.locb` to .gitignore to make our current temporary decision explicit, otherwise it could confuse a new developer checking out the code and running the tests for the first time preparing to submit a small branch PR, just to get going. (that's how I found the lock file and this thread by the way.)
ie. i was suddenly forced to have to make a decision whether to ignore the changed file. The first thing I did was google, should it be included, and the results were quiet biased towards including it. If we exclude it with a .gitignore we wont have to worry about it, until something actually happens, which will give us a real scenario that we can ensure any changes supports pragmatically.

New devs to the project running Bun test for the first time will just see green. This follows the Principle of least suprise.

@nobkd
Copy link
Collaborator

nobkd commented Feb 1, 2024

Configure git to show the diff & other details like clear text lock-files:

https://bun.sh/docs/install/lockfile (details, git, etc.)
https://bun.sh/guides/install/yarnlock (additional human-readable file (also included above to some extent))


Also see this comments here:

#150 (comment) (general comment)
#150 (comment) (concerns)
#150 (comment) (faster installs)


But I can also see your concerns regarding new developers and binary lock-files in general.

Maybe we could add to the developer guidelines, that the lock file should be used for installation (, also in tests on this end)?
The lock-file would have to be updated now and then, in this case (an action that runs periodically? e.g., every week?). 🤷
bun install --frozen-lockfile

Just thinking aloud...

@goblinfactory
Copy link
Contributor

goblinfactory commented Feb 1, 2024

@nobkd To solve the problem of "least suprise" it would also be quite ok to simply also check in the lock file, assuming that if I run a build bun would generate the same deterministic binary lockfile. easy enough to test; if that works that also makes the scary "why am i suddenly seeing a changed file that should be under source control" warning.

i.e. I should have mentioned above another simple solution is also to just check the file in to source control.

Thanks for the links to #150 those were quite helpful. Bun looks quite smart, and I can totally understand why the build is so much faster, makes sense when you read the explanations.

My "thumbs up" above isn't me saying we should cron job updating lock-file; I need to get more /deeper involved in the project before I can comment otherwise I risk saying stuff without important context and understanding. My thumbs up means I think some discussion around your idea is valuable.

Doing nothing, or the simplest and smallest something, (like, just checking it in) will at the very least give us more information (data) to make a better decision on.

@tipiirai
Copy link
Contributor

tipiirai commented Feb 5, 2024

@goblinfactory let's talk about this tomorrow!

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.

4 participants