vikejs / vike

🔨 Flexible, lean, community-driven, dependable, fast Vite-based frontend framework.
https://vike.dev
MIT License
4.39k stars 352 forks source link

Eject #1957

Closed snake-py closed 1 week ago

snake-py commented 2 weeks ago

Somehow it closed my open PR after I had to reset my fork to Vike's current state.

@brillout I think I implemented all of your last comments. Sorry that I took so long to work on this. How did you test, that you came to this commit?

If I use this package like here, I get this commit.

See old PR.

brillout commented 2 weeks ago

:+1: Nice, looking forward to try your new version. I get some build errors:

/code/vike/packages/eject (pr/1957-Eject$) pnpm run build

> eject@0.0.1 build /home/rom/code/vike/packages/eject
> tsc

src/command.ts:25:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

25   console.log('Ejected dependencies:', successFullEjections)
     ~~~~~~~

src/git.ts:1:26 - error TS2307: Cannot find module 'child_process' or its corresponding type declarations.

1 import { execSync } from 'child_process'
                           ~~~~~~~~~~~~~~~

src/index.ts:16:11 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

16 cli.parse(process.argv.length === 2 ? [...process.argv, '--help'] : process.argv)
             ~~~~~~~

src/index.ts:16:43 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

16 cli.parse(process.argv.length === 2 ? [...process.argv, '--help'] : process.argv)
                                             ~~~~~~~

src/index.ts:16:69 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

16 cli.parse(process.argv.length === 2 ? [...process.argv, '--help'] : process.argv)
                                                                       ~~~~~~~

src/index.ts:18:1 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

18 process.on('unhandledRejection', (rejectValue) => {
   ~~~~~~~

src/index.ts:18:35 - error TS7006: Parameter 'rejectValue' implicitly has an 'any' type.

18 process.on('unhandledRejection', (rejectValue) => {
                                     ~~~~~~~~~~~

src/log.ts:4:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

4   console.log(chalk.green(message, optionalParams))
    ~~~~~~~

src/log.ts:8:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

8   console.log(chalk.red(message, optionalParams))
    ~~~~~~~

src/log.ts:12:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

12   console.log(chalk.blue(message, optionalParams))
     ~~~~~~~

src/log.ts:16:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

16   console.log(chalk.yellow(message, optionalParams))
     ~~~~~~~

src/system.ts:1:25 - error TS2307: Cannot find module 'path' or its corresponding type declarations.

1 import { resolve } from 'path'
                          ~~~~~~

src/system.ts:2:16 - error TS2307: Cannot find module 'fs' or its corresponding type declarations.

2 import fs from 'fs'
                 ~~~~

src/system.ts:3:26 - error TS2307: Cannot find module 'child_process' or its corresponding type declarations.

3 import { execSync } from 'child_process'
                           ~~~~~~~~~~~~~~~

src/system.ts:51:17 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

51   const agent = process.env.npm_config_user_agent
                   ~~~~~~~

src/system.ts:61:7 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

61   if (process.env.npm_execpath && process.env.npm_execpath.endsWith('yarn.js')) {
         ~~~~~~~

src/system.ts:61:35 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

61   if (process.env.npm_execpath && process.env.npm_execpath.endsWith('yarn.js')) {
                                     ~~~~~~~

src/system.ts:66:7 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

66   if (process.env.npm_command) {
         ~~~~~~~

src/system.ts:71:18 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

71   const parent = process.env._
                    ~~~~~~~

Found 19 errors in 5 files.

Errors  Files
     1  src/command.ts:25
     1  src/git.ts:1
     5  src/index.ts:16
     4  src/log.ts:4
     8  src/system.ts:1
 ELIFECYCLE  Command failed with exit code 2.
snake-py commented 2 weeks ago

@brillout this is odd, I just tried the same and for me it is building fine.

code ~/Desktop/playground/vike-1553/packages/eject

pnpm build

> eject@0.0.1 build /home/fabio/Desktop/playground/vike-1553/packages/eject
> tsc

// no output just the finished build

to me it seems like you have not ran pnpm i maybe? Or did I forget something?

brillout commented 2 weeks ago
~/code/vike (pr/1957-Eject$) pnpm run reset

> @ reset /home/rom/code/vike
> git clean -Xdf && pnpm install && pnpm run build

Removing .github/workflows/ci/node_modules/
Removing boilerplates/boilerplate-react-ts/node_modules/
Removing boilerplates/boilerplate-react/node_modules/
Removing boilerplates/boilerplate-vue-ts/node_modules/
Removing boilerplates/boilerplate-vue/node_modules/
Removing boilerplates/node_modules/
Removing docs/node_modules/
Removing examples/auth/node_modules/
Removing examples/base-url-cdn/node_modules/
Removing examples/base-url-server/node_modules/
Removing examples/base-url/node_modules/
Removing examples/cloudflare-workers-react-full/node_modules/
Removing examples/cloudflare-workers-react/node_modules/
Removing examples/cloudflare-workers-vue/node_modules/
Removing examples/custom-preload/node_modules/
Removing examples/file-structure-domain-driven/node_modules/
Removing examples/html-fragments/node_modules/
Removing examples/i18n/node_modules/
Removing examples/path-aliases/node_modules/
Removing examples/react-full/dist/
Removing examples/react-full/node_modules/
Removing examples/react-minimal/node_modules/
Removing examples/react-streaming/node_modules/
Removing examples/render-modes/node_modules/
Removing examples/telefunc/node_modules/
Removing examples/vue-full/node_modules/
Removing examples/vue-minimal/node_modules/
Removing node_modules/
Removing packages/eject/dist/
Removing packages/eject/node_modules/
Removing test-deprecated-design/base-url-cdn/node_modules/
Removing test-deprecated-design/base-url-server/node_modules/
Removing test-deprecated-design/base-url/node_modules/
Removing test-deprecated-design/cloudflare-workers-react-full/node_modules/
Removing test-deprecated-design/cloudflare-workers-react/node_modules/
Removing test-deprecated-design/cloudflare-workers-vue/node_modules/
Removing test-deprecated-design/file-structure-domain-driven/node_modules/
Removing test-deprecated-design/html-fragments/node_modules/
Removing test-deprecated-design/i18n/node_modules/
Removing test-deprecated-design/path-aliases/node_modules/
Removing test-deprecated-design/react-full/node_modules/
Removing test-deprecated-design/react-streaming/node_modules/
Removing test-deprecated-design/react/node_modules/
Removing test-deprecated-design/render-modes/node_modules/
Removing test-deprecated-design/telefunc/node_modules/
Removing test-deprecated-design/vue-full/node_modules/
Removing test-deprecated-design/vue/node_modules/
Removing test/abort/node_modules/
Removing test/assertFileEnv/node_modules/
Removing test/cjs/node_modules/
Removing test/disableAutoImporter/node_modules/
Removing test/env/node_modules/
Removing test/hook-override/node_modules/
Removing test/includeAssetsImportedByServer/node_modules/
Removing test/playground/node_modules/
Removing test/preload/node_modules/
Removing test/require-shim/node_modules/
Removing test/stream-vue-onServerPrefetch/node_modules/
Removing test/vike-react/node_modules/
Removing test/vike-vue/node_modules/
Removing test/webpack/node_modules/
Removing vike/dist/
Removing vike/node_modules/
Scope: all 60 workspace projects

devDependencies:
+ @biomejs/biome 1.7.3
+ @brillout/test-e2e 0.5.36
+ @brillout/test-types 0.1.14
+ playwright 1.47.0
+ prettier 3.3.0
+ vitest 2.1.3

Done in 6.7s

> @ build /home/rom/code/vike
> cd vike/ && pnpm run build

> vike@0.4.201 build /home/rom/code/vike/vike
> rimraf dist/ && pnpm run build:esm && pnpm run build:cjs

> vike@0.4.201 build:esm /home/rom/code/vike/vike
> tsc

> vike@0.4.201 build:cjs /home/rom/code/vike/vike
> pnpm run build:cjs:ts && pnpm run build:cjs:fixup

> vike@0.4.201 build:cjs:ts /home/rom/code/vike/vike
> tsc --project ./tsconfig.cjs.json

> vike@0.4.201 build:cjs:fixup /home/rom/code/vike/vike
> node ./dist-cjs-fixup.mjs

✅ dist/cjs/package.json generated
✅ dist/cjs/ shimmed import.meta.url
~/code/vike (pr/1957-Eject$) cd packages/eject/
bin-entry.js  node_modules  package.json  src  tsconfig.json
~/code/vike/packages/eject (pr/1957-Eject$) pnpm run build

> eject@0.0.1 build /home/rom/code/vike/packages/eject
> tsc

src/command.ts:25:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

25   console.log('Ejected dependencies:', successFullEjections)
     ~~~~~~~

src/git.ts:1:26 - error TS2307: Cannot find module 'child_process' or its corresponding type declarations.

1 import { execSync } from 'child_process'
                           ~~~~~~~~~~~~~~~

src/index.ts:16:11 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

16 cli.parse(process.argv.length === 2 ? [...process.argv, '--help'] : process.argv)
             ~~~~~~~

src/index.ts:16:43 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

16 cli.parse(process.argv.length === 2 ? [...process.argv, '--help'] : process.argv)
                                             ~~~~~~~

src/index.ts:16:69 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

16 cli.parse(process.argv.length === 2 ? [...process.argv, '--help'] : process.argv)
                                                                       ~~~~~~~

src/index.ts:18:1 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

18 process.on('unhandledRejection', (rejectValue) => {
   ~~~~~~~

src/index.ts:18:35 - error TS7006: Parameter 'rejectValue' implicitly has an 'any' type.

18 process.on('unhandledRejection', (rejectValue) => {
                                     ~~~~~~~~~~~

src/log.ts:4:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

4   console.log(chalk.green(message, optionalParams))
    ~~~~~~~

src/log.ts:8:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

8   console.log(chalk.red(message, optionalParams))
    ~~~~~~~

src/log.ts:12:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

12   console.log(chalk.blue(message, optionalParams))
     ~~~~~~~

src/log.ts:16:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

16   console.log(chalk.yellow(message, optionalParams))
     ~~~~~~~

src/system.ts:1:25 - error TS2307: Cannot find module 'path' or its corresponding type declarations.

1 import { resolve } from 'path'
                          ~~~~~~

src/system.ts:2:16 - error TS2307: Cannot find module 'fs' or its corresponding type declarations.

2 import fs from 'fs'
                 ~~~~

src/system.ts:3:26 - error TS2307: Cannot find module 'child_process' or its corresponding type declarations.

3 import { execSync } from 'child_process'
                           ~~~~~~~~~~~~~~~

src/system.ts:51:17 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

51   const agent = process.env.npm_config_user_agent
                   ~~~~~~~

src/system.ts:61:7 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

61   if (process.env.npm_execpath && process.env.npm_execpath.endsWith('yarn.js')) {
         ~~~~~~~

src/system.ts:61:35 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

61   if (process.env.npm_execpath && process.env.npm_execpath.endsWith('yarn.js')) {
                                     ~~~~~~~

src/system.ts:66:7 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

66   if (process.env.npm_command) {
         ~~~~~~~

src/system.ts:71:18 - error TS2580: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.

71   const parent = process.env._
                    ~~~~~~~

Found 19 errors in 5 files.

Errors  Files
     1  src/command.ts:25
     1  src/git.ts:1
     5  src/index.ts:16
     4  src/log.ts:4
     8  src/system.ts:1
 ELIFECYCLE  Command failed with exit code 2.
~/code/vike/packages/eject (pr/1957-Eject$) 
snake-py commented 2 weeks ago

@brillout So this is the first time working in a monerepo. I guess for me it was working because I wrongly used pnpm. After I ran pnpm reset I als had the same error. To resolve this I installed @types/node, I am not sure if this is the right way though?

For me build works now, but I have not tested the ejectjs command within a monorepo structure in the respect of symlinks.

brillout commented 2 weeks ago

It builds now :+1:

Let me try it :eyes:

brillout commented 2 weeks ago

I can reproduce the bug:

~/code/vike/examples/react-full (pr/1957-Eject$) pnpm eject node-fetch
Checking git history 
Ejected dependencies: [ 'node-fetch' ]
We updated your package.json and linked the dependencies to the ejected folder 
finalize the ejection by running install 
Committing ejection 
Ejection committed with message: eject vike-react [done by https://npmjs.com/package/eject]
~/code/vike/examples/react-full (pr/1957-Eject*$) git status
On branch pr/1957-Eject
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   ../../pnpm-lock.yaml

no changes added to commit (use "git add" and/or "git commit -a")
~/code/vike/examples/react-full (pr/1957-Eject*$) git show HEAD
commit e22fd6dbb7df544184d7b170f8033863f6360fee (HEAD -> pr/1957-Eject)
Author: Romuald Brillout <git@brillout.com>
Date:   Fri Nov 8 12:10:02 2024 +0100

    eject vike-react [done by https://npmjs.com/package/eject]

diff --git a/examples/react-full/ejected/node-fetch b/examples/react-full/ejected/node-fetch
new file mode 120000
index 0000000000..864389f9a7
--- /dev/null
+++ b/examples/react-full/ejected/node-fetch
@@ -0,0 +1 @@
+/home/rom/code/vike/node_modules/.pnpm/node-fetch@3.3.2/node_modules/node-fetch
\ No newline at end of file
diff --git a/examples/react-full/package.json b/examples/react-full/package.json
index 0ef4029022..b77ebaaa1f 100644
--- a/examples/react-full/package.json
+++ b/examples/react-full/package.json
@@ -15,7 +15,7 @@
     "@vitejs/plugin-react-swc": "^3.6.0",
     "cross-fetch": "^4.0.0",
     "eject": "0.4.0",
-    "node-fetch": "^3.3.2",
+    "node-fetch": "file:./ejected/node-fetch",
     "react": "^18.2.0",
     "react-dom": "^18.2.0",
     "react-streaming": "^0.3.43",
@@ -23,5 +23,6 @@
     "vike": "0.4.201",
     "vite": "^5.4.0"
   },
-  "type": "module"
-}
+  "type": "module",
+  "devDependencies": {}
+}
\ No newline at end of file
brillout commented 2 weeks ago
brillout commented 2 weeks ago

The package manager detection is neat. I think we can even improve it: how about we search for the lock files of each pacakge manager (e.g. pnpm-lock.json for pnpm and package-lock.json for npm) and use that to determine which package manager to use? I think that's a lot more reliable and I'd even say that we should skip the install if we can't find a lock file.

snake-py commented 2 weeks ago

@brillout For me it is creating this commit. I also did a tiny adjustment, so now symlinks are copied without reference.

To achive this I had to add to examples/react-full/package.json the following script "eject": "ejectjs".

Then running

cd ~/Desktop/playground/vike-1553/examples/react-full
pnpm eject node-fetch

I think I named the command ejectjs, because I had a naming collision with https://manpages.ubuntu.com/manpages/trusty/man1/eject.1.html as I was trying to register it globally with eject.

I am now going to implement the other suggestion you had.

snake-py commented 2 weeks ago

The package manager detection is neat. I think we can even improve it: how about we search for the lock files of each pacakge manager (e.g. pnpm-lock.json for pnpm and package-lock.json for npm) and use that to determine which package manager to use? I think that's a lot more reliable and I'd even say that we should skip the install if we can't find a lock file.

I think this makes sense, however I am unsure how this would then behave in a monorepo structure? I mean I am runing the command inside ./examples/react-full and there is no lock file? But I see that the current logic is not really suitable.

brillout commented 1 week ago

I think this makes sense, however I am unsure how this would then behave in a monorepo structure? I mean I am runing the command inside ./examples/react-full and there is no lock file? But I see that the current logic is not really suitable.

I believe it works reliably if we walk up the filesystem tree and abort as soon as we stumble upon a lock file. The first lock file we encounter determines the package manager. I can't think of a (non-contrived) situation where this would be wrong. It seems to be a lot more reliable than just checking the package managers the user installed.

For me it is creating this commit.

Neat, let me try agian.

brillout commented 1 week ago

I can confirm it now ejects the code. Not only that but it actually works: the node-fetch is fully ejected and the app still works. That's awesome :star_struck:

Let's finish the polishing and then release a first version. How about we then create a new GitHub repository for it? At https://github.com/snake-py/eject or https://github.com/vikejs/eject — whichever you prefer.

snake-py commented 1 week ago

@brillout sure I will add the final part where I update the dependency manager detection. I also thought of using maybe some env variable? maybe pnpm, npm bun are setting some envs during their script runs. Then we would now for sure what was used? I will play around with this.

About the release repository, maybe it depends on what the vision here is.

I see it more of a stand a lone thing, and less a vike thing. But we want to have first class support for vike packages. I guess we could offer as I stated here a generic approach.

brillout commented 1 week ago

@brillout sure I will add the final part where I update the dependency manager detection. I also thought of using maybe some env variable? maybe pnpm, npm bun are setting some envs during their script runs. Then we would now for sure what was used? I will play around with this.

Good idea :+1:

About the release repository, maybe it depends on what the vision here is.

I see it more of a stand a lone thing, and less a vike thing. But we want to have first class support for vike packages. I guess we could offer as I stated here a generic approach.

Yes, I think we can make it completely agnostic to Vike.

snake-py commented 1 week ago

@brillout I went now with the simple solution of looking for the lock file. Please have a look in the new repository. I also added you there.

brillout commented 1 week ago

@snake-py :100: I'll be creating an issue for polishing potential. I'd also suggest using pnpm and Biome with Vike's style (but let me know if you prefer owning stylistic decisions instead; you now own the eject package after all :slightly_smiling_face:)

snake-py commented 1 week ago

@brillout I don't know Biome. I do have my own opinions, but if there is a vite community standard I would follow it. I will have a look at Biome. But for now I guess we can release the first version. What do you think?

but let me know if you prefer owning stylistic decisions instead;

Your input is appreciated, it is my first real open source development.

snake-py commented 1 week ago

I have opened the polishing issue.

brillout commented 1 week ago

I do have my own opinions

:100: I'm up for making it your project and I just give recommendations. Feel free to reject/ignore any recommendation I give. I may directly commit changes if it's nothing major and consensual (feel free to push back on that as well).

pnpm is very widespread in Vite's ecosystem, so yea I'd recommend pnpm. That said for a project without many dependencies I guess npm is okay. For the test/ folder we may want to have a monorepo structure and I ain't sure how well npm's monorepo support is. For now I think we can skip testing altogether.

As for Biome it's a lot faster (and also more feature rich but I personally don't need these extra features). That said, prettier is fine I guess.

One other thing I'd recommend is using https://github.com/alexeyraspopov/picocolors instead of chalk. It's what Vite uses; it's a lot more lightweight.

brillout commented 1 week ago

Closing as the code now lives at https://github.com/snake-py/eject.