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

Draft: Eject #1798

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Draft: Eject #1798

wants to merge 12 commits into from

Conversation

snake-py
Copy link

@snake-py snake-py commented Aug 10, 2024

I started to work on the eject command. Since I don't know vike-react-apollo I took a more generic route. I am unsure if this maybe also solves the need. Basically the command copies everxthing form the dependency over into a created ejectedfolder and then links package.json to that folder. If this fits the needs as well I would go ahead and finalize this. Maybe someone can give some feed back as well?

Prior art:

@brillout
Copy link
Member

updatedDependencies[key] = packageJson.dependencies[`file:./ejected/${key}`]

Neat idea. Does it work?

If this fits the needs as well I would go ahead and finalize this.

If it works then, yes, that'd be great!

@brillout brillout changed the title Worked on 1553 Eject Aug 11, 2024
@brillout
Copy link
Member

Does it work for all package managers? E.g. Yarn, pnpm, Deno, and Bun.

In general, do we foresee any potential issue?

One drawback of this approach is that it doesn't eject the TypeScript source code. Maybe we can go for of a dual approach: some packages (e.g. Vike extensions) have first-class support for eject (e.g. ejecting TypeScript) while other packages fallback to using this node_modules/ copying approach. WDYT?

@snake-py
Copy link
Author

Does it work for all package managers? E.g. Yarn, pnpm, Deno, and Bun.

I''l check on that.

One drawback of this approach is that it doesn't eject the TypeScript source code. Maybe we can go for of a dual approach: some packages (e.g. Vike extensions) have first-class support for eject (e.g. ejecting TypeScript) while other packages fallback to using this node_modules/ copying approach. WDYT?

Yes you are right. How about a flag --source. If set I go and download the original source code (I guess the simplest way is to detect the git repository and then try to download it from github/gitlab...). Still place in the ejected folder. I guess for exports I could then generate a package.json which export the ts files. As an alternative we could suggest to update vite config with an alias.

@brillout
Copy link
Member

👍 We can try that and see how it goes. (We'll probably stumble upon some issues but we may be able to work around all of them.)

@snake-py
Copy link
Author

snake-py commented Aug 12, 2024

@brillout I added a new commit and I will need some feedback here on how to proceed. So as I understand now, the goal is basically to eject only dependencies from https://github.com/brillout/vike-apollo-react. I adjusted the coding a little bit and for now it would only work with this repo (which I think is a downside).

The current command ejects the dependency into the ejected folder:
image

The questions I have:

  1. Is the limitation fine that it only works with the monorepo for now?
  2. What is the prefered way here? Would you rather have files stay in ejected area and have the user adjust their vike.config?
  3. Should I also update package.json to install required dependencies?
  4. I also thought of having maybe some file inside each package do this (scaffoldProject.js). Then I basically pull everything and the script of a module would take over. I think the developer of the module knows best what files would need to be copied where.

@snake-py snake-py changed the title Eject Draft: Eject Aug 12, 2024
import { resolve } from 'path'
import { Readable } from 'stream'
import { finished } from 'stream/promises'
// const response = await fetch(NPM_REGISTRY_BASE_URL + dependency)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First thought of doing a more generic approach. However, this would let me rely on some source to get the information where to find the source files. So I went ahead and restricted it for now to only work with the mono-repo. I think we could maybe add some logic based on what repo?

Comment on lines 3 to 5
import { prerender } from './commands/prerender.js'
import { eject } from './commands/eject.js'
import { scaffold } from './commands/scaffold.js'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It felt odd to have everything in bin.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, I agree, but let's discuss this kind of things later. This will actually heavily conflict with what I'm currently working on (the Vike CLI fully replacing Vite's CLI) so I'm inclined to think it's best we revert this refactor.

@nitedani
Copy link
Member

nitedani commented Aug 12, 2024

We can store extra needed info under the vike-eject key in each extension.

// package.json

"vike-eject": {
 "exports": {
    "./something" : "./src/something.ts"
 },
 "repo": "https://github.com/vikejs/vike-react/tree/main/packages/vike-react-query"
}

User runs pnpm vike eject vike-react-query
Then use this url for downloading the source
Put it into ejected/vike-react-query
Then we can rewrite package.json, using "vike-eject".exports
Detect package manager
Link the package in the users package.json
Run install script

import { execSync } from 'child_process'

function detectPackageManager() {
  // Check npm_config_user_agent first
  const agent = process.env.npm_config_user_agent
  if (agent) {
    const [program] = agent.toLowerCase().split('/')
    if (program === 'yarn') return 'yarn'
    if (program === 'pnpm') return 'pnpm'
    if (program === 'npm') return 'npm'
    if (program === 'bun') return 'bun'
  }

  // Check npm_execpath for yarn
  if (process.env.npm_execpath && process.env.npm_execpath.endsWith('yarn.js')) {
    return 'yarn'
  }

  // Check npm_command for npm
  if (process.env.npm_command) {
    return 'npm'
  }

  // Check _ environment variable as a last resort
  const parent = process.env._
  if (parent) {
    if (parent.endsWith('pnpx') || parent.endsWith('pnpm')) return 'pnpm'
    if (parent.endsWith('yarn')) return 'yarn'
    if (parent.endsWith('npm')) return 'npm'
    if (parent.endsWith('bun')) return 'bun'
  }

  // If all else fails, assume npm
  return 'npm'
}

const packageManager = detectPackageManager()
execSync(`${packageManager} install`)

@snake-py
Copy link
Author

snake-py commented Aug 12, 2024

@nitedani thanks for the input

We can store extra needed info under the vike-eject key in each extension.
This makes a lot of sense and would make the development simpler. However, I don't see the need to store the url there. The main issue is basically that I need to have the URL available to even find the package.json of the extension. So we could also maintain some sort of available extensions within the CLI.

// would be stored close to the cli. Then we can maintain official extensions and they respective download paths
const extensions = {
  "vike-react-apollo: "download_path"
}

I like the idea of having a vike key inside the extension's package.json so the module developer can have some controll on how the extension is ejected. Maybe something between the lines of:

// inside extension package.json
  "vike": {
    "cli": {
      "eject": {
        "exports": {}, // the new src exports so the file:./link works
        "remove": {}, // files or folder which should be removed
        "customScaffoldScript": "path_to_some_script which runs after the ejection",
         // should maybe only work for trusted extensions, we might want to include trust level in the top list, WDYT?
        "integratePaths": {
          "pathInExtension": "pathInScaffoldedProject" // maybe for files which should directly be copied to renderer
        },
        "installDependencies": true
      }
    }
  }

@nitedani
Copy link
Member

nitedani commented Aug 12, 2024

The main issue is basically that I need to have the URL available to even find the package.json of the extension.

You can use require.resolve or import.meta.resolve to get the path.

import { createRequire } from 'node:module';
import { join } from 'node:path';

const require_ = createRequire(import.meta.url);
const packagePath = require_.resolve('vike-react-query');
const packageJsonPath = join(packagePath, 'package.json');

"installDependencies": true

The dependencies always need to be installed, the package wouldn't work without that.
But if we are ejecting a package it means that it was already installed before, so maybe we can skip the install step, because the dependencies should already be there.. so you're right, maybe install is not needed. But to update the lockfile we need to run install.

For installation of dependencies I think you can just run the install command in process.cwd after patching the users package.json (also patch other already installed dependent extensions package.json)

But how to find the dependent packages? For example vike-react-query depends on vike-react. If we eject vike-react, we need to patch not only the users package.json but also vike-react-query package.json to link ejected/vike-react. Maybe this wouldn't even work with yarn.. We can parse the lockfile though and find the dependents.

In monorepo usually there is a single instance of an installed package, and its symlinked to multiple apps, so to avoid the wrong packages being loaded, we would need to eject all dependents to the local app/ejected directory. So if we eject vike-react we also need to eject vike-react-query, otherwise other apps that depend on vike-react-query in the same monorepo would also load the ejected vike-react.. Don't we? Maybe only in a monorepo?

otherwise other apps that depend on vike-react-query in the same monorepo would also load the ejected vike-react

Or we can make this behavior explicit by ejecting to the monorepo root/ejected directory.

"customScaffoldScript": "path_to_some_script which runs after the ejection",
"integratePaths": {
"pathInExtension": "pathInScaffoldedProject" // maybe for files which should directly be copied to renderer
}

I wouldn't add these for now, let's see if it's really needed and we can add it later if it is.

@snake-py
Copy link
Author

You can use require.resolve or import.meta.resolve to get the path.

I guess, I wanted to get rid of the constraint that the user would need to install the package first. But I guess you are right, the constraint seems logical and makes developing much easier.

The dependencies always need to be installed, the package wouldn't work without that.
But if we are ejecting a package it means that it was already installed before, so maybe we can skip the install step, because the dependencies should already be there.. so you're right, maybe install is not needed. But to update the lockfile we need to run install.

Yeah, I was just not sure if we want to run the install command. I would prefer to simply set everything up and then tell the user to run the install command. Then they would have some time to review the made changes. But Either way works for me.

But how to find the dependent packages?

I honestly don't think that this will be an issue. However, I know for npmthat I could add the ejected dependency to overrrides. Is this what you meant?

For example vike-react-query depends on vike-react

Honestly, this depends on what we are trying to achieve here. Looking at the original discussion, the need sounded to me more like scaffolding some pages or +config. I think a first step would now to not respect peer dependencies and let the dev deal with it? Is this even something we should automate? If someone goes that far to eject these dependencies, they are probably going to make considerable changes to the code and maybe at the end they are not relying anymore on the peer dependency.

In monorepo usually there is a single instance of an installed package, and its symlinked to multiple apps, so to avoid the wrong packages being loaded, we would need to eject all dependents to the local app/ejected directory.

I guess, this is what I would have done anyway 🤔 So basically I would expect that the commands runs at the location where the package.json is which we want to update. Within the same dir I would create the ejected folder. If the person now runs this at root level package.json I would eject the dependency for all apps within the monorepo and update the main package.json

@snake-py
Copy link
Author

snake-py commented Aug 13, 2024

WIth the updated code I now have in my test project:

image
image
image
image

And inside the test project I imported the component like this:

image

btw, I usually don't work so much with GitHub, is there a proper way to mark this PR as draft, so that I am not running all checks on every commit I am doing?

@snake-py
Copy link
Author

@nitedani or @brillout a review of this would be great, I kinda need some input if this is now what is needed. Then I can maybe clean up the code and implement the last missing function as well as maybe unit tests?

@brillout
Copy link
Member

brillout commented Sep 5, 2024

I've given this a little bit more thoughts. How about the following?

We have different levels of support:

  1. [Level 1] Supports any npm package: it copies JavaScript from node_modules/some-package/ then uses the file: protocol.
  2. [Level 2] Supports any npm package that defines a basic eject config at node_modules/some-package/package.json#eject.
  3. [Level 3] Supports any npm package that defines an advanced eject config at node_modules/some-package/package.json#eject (or node_modules/some-pacakge/eject.config.js if JavaScript is needed).

I don't know what the differences between level 2 and 3 would be, and maybe we will end up with only level 1 and 2.

Couple of notes:

  • Most of it is agnostic to Vike, so we could eventually write an eject specification that is agnostic to Vike. Maybe we can go ahead and use the package.json#eject property.

    // package.json
    {
      "eject": {
        // Some unique name + version
        "version": "awesome-eject@1",
        // Maybe, in the future, we could declare a schema
        "$schema": "./node_modules/eject/schema.json",
        "exports": {
          ".": "./dist/index.js"
        },
        "remove": ["./CHANGELOG.md"]
      }
    }
  • Can we make level 1 work for any npm package out there? So far, it seems like it's possible.

  • we might want to include trust level in the top list, WDYT?

    Trust isn't an issue, because as soon as you npm install some-package then you fully trust some-package anyways. So we can go ahead and assume some-package to be a good citizen.

  • Some prior art here which includes a couple of goodies we can eventually copy. (E.g. enforcing the user to have a clean Git state before ejecting, and then making a Git commit after ejecting.)

Couple of features we could implement in the future:

  • Partial eject. This will be important as we iterate towards the Stem vision. For example, for Vike auth extensions that automatically implement the /login and /password-reset pages, it would be nice to be able to eject only the /login and /password-reset pages without ejecting the whole extension.

  • Remove ejected/some-package/tsconfig.json in favor of using the one defined by the user. I guess this will be tricky to achieve, so I'm inclined to postpone this for now. (Although we can already start experiementing if we want.)

  • maybe for files which should directly be copied to renderer

    That would be awesome, but I think this is tricky to achieve because the TypeScript configuration needs to be similar. So maybe we should postpone this.

  • Conditions: I think some advanced eject functionalities will require some conditions. Such as the last two points: the user must use TypeScript and must have a similar tsconfig.json.

  • Maybe we can enforce certain things about tsconfig.json for ejectable packages and for the user. If the user respects these tsconfig.json constraints then this unlocks new eject functionalities.

But let's focus on the basics first. We can improve things later in subsequent PRs.

Apologies for the delayed answer, I was busy with a couple of things. @snake-py if you're still up for it, then let's try to merge something not-perfect-but-functional relatively quickly.

  • I think a first step would now to not respect peer dependencies and let the dev deal with it?

    I think we can ignore this for now (or we just show a warning). But, yeah, in the future we could do something clever about peer dependencies (e.g. chaining ejects).

a review of this would be great, I kinda need some input if this is now what is needed. Then I can maybe clean up the code and implement the last missing function as well as maybe unit tests?

Let me know if you need more input. Looking forward to merge eject 😍

@brillout
Copy link
Member

I managed to be the owner of https://www.npmjs.com/package/eject. Ideally, long term, we will be able to make our work Vike agnostic.

@snake-py
Copy link
Author

@brillout sorry for not answering, just very busy at the moment. If you really think it makes sense to go forward with the current work. I can finalize a MR. However, I also would be willing to contribute in a different way in case it is more productive for the project.

In case I can proceed with the current could you maybe give me a clear scope how far I should go? I am a little confused by what eject now should and should not do?

@brillout
Copy link
Member

How about we first try something relatively simple, for example:

[Level 1] Supports any npm package: it copies JavaScript from node_modules/some-package/ then uses the file: protocol.

In other words, we don't fetch the TypeScript source.

I think you already got something working? Is there something missing?

I suggest we first finish this relatively simple solution, then let's see what we want to do next. Maybe we can then try fetching the TypeScript source in a subsequent PR. Or maybe we'll see some other neat things we want to polish about eject.

WDYT?

To be clear: I think eject should eventually support all of these options:

  1. Moving code from node_modules/ (level 1).
    • The moved code is already built ✅
    • Can we make this work for any npm package?
  2. Fetching TypeScript (or JavaScript) source code (level 2).
    • This is more complex as the code needs to be built.
    • I expect this to work only for npm packages that explicitly support eject.
  3. More advanced stuff (partial eject, re-using the user's tsconfig.json, ...)
    • This will require the npm package to define an eject configuration and/or comply with certain rules.

But let's start with only supporting level 1.

The reason I think it makes sense to simultaneously support all these levels is because npm packages will work only with a certain level.

@snake-py
Copy link
Author

snake-py commented Sep 21, 2024

@brillout alright, yes I have the coding for the level-1 already ready. I updated the code to include only the level-1 support.

How it works:

  1. Add the following script to a project where vike is installed
    "eject": "vike eject"
  1. run npm run eject react react-dom and list as much dependencies as you want.
  2. You will need to run npm i manually to finalize the ejection.

Please let me know if this suitable for first level.

@brillout
Copy link
Member

Very neat 💯

Actually, how about we go ahead and define it as eject npm package? Let's keep the source code in this repo for now, inside packages/eject/ (see pnpm monorepos).

Let me know your npm handle and I'll make you co-owner of the eject package.

  • Some prior art here which includes a couple of goodies we can eventually copy. (E.g. enforcing the user to have a clean Git state before ejecting, and then making a Git commit after ejecting.)

I think we can/should already do this, even for level 1 (for all levels really). It's quite a scary operation, so I'm inclined to think it's best we enforce the user to have a clean Git state.

You will need to run npm i manually so finalize the ejection.

WDYT of running it on behalf of the user, as shown here? In general, there are pros and cons about doing this, but I think for eject it's worth it.


I'll further review the code tomorrow.

@snake-py
Copy link
Author

Sure I am following your lead there. I think you are right and it makes sense to have it as a stand alon package since, it is not really directly vike related. My npm profile name is the same as my GitHub https://www.npmjs.com/~snake-py

I can implement both changes to the eject package then. I need to have a look around first. Will try to do it this week.

@brillout
Copy link
Member

👍

$ npm owner add snake-py eject
npm notice INFO: User snake-py invited to package eject successfully.
+ snake-py (eject)

You should have received an invitation email.

@snake-py
Copy link
Author

Alright, I have not much experiences with public npm packages. I have some basic org question.

I see that currently this eject is providing the source code to the npm package.
Your prior work is in your eject repo.

Will we keep the bigeasy's repo, or do you think we want to reconnect to yours? Or should I create a new one? Either way can you maybe make me maintainer or should I fork and do the changes in a fork?

@brillout
Copy link
Member

I think it's easier for now if we make the vikejs/vike repository a monorepo and define the source code of the eject package at packages/eject/ (while keeping the Vike source code where is at vike/). We don't have to release now, we can release later as we can develop without releasing (see the monorepo documentation).

@snake-py
Copy link
Author

@brillout I hope I understood you correctly.

I updated the code:

  • moved the eject command to ./packages/eject
  • added the coding for checking git
  • added the install step

I had trouble to get it to work with just using eject as command. It seems to collide with brew's eject command. I don't know why exactly, that is why I renamed it to ejectjs.

How it works:

  1. Add the following script to a project where vike is installed
    "eject": "ejectjs"
  1. run npm run eject express and list as much dependencies as you want.
  2. You will need to run npm i manually to finalize the ejection.

Pitfalls

  • Current logic requires the package name same to the folder name (I had an odd case where it was reproducible, but it went away)
  • no unit tests (but can add)
  • dependencies can only be ejected once

@snake-py
Copy link
Author

snake-py commented Sep 29, 2024

@brillout I just noticed that my last comment was not completely correct. Probably because I copied over from a previous one. The install is done for the user automatically. I added some flags to bypass git check or skip the install.

@brillout
Copy link
Member

Ok 👍

I just tried it, and it generates this commit. I guess there is a bug?

@brillout
Copy link
Member

Couple of notes:

  • How about we improve the commit message, e.g. eject vike-react [done by https://npmjs.com/package/eject].
  • If Git isn't installed, then let's abort and tell the user to use --force.
  • How about we add --skip-commit actually I don't think it's needed: correct me if I'm wrong, but I don't see a use case for it and the user can always $ git reset HEAD~ anyways.
  • Do we foresee a use case for --skip-install?
  • How about we remove --message: the user can $ git commit --amend instead.
  • I ain't sure about using simple-git. It adds 1.7 MB which is non-negligble for what it does. I find using a middleman more confusing than adding benefits. E.g. https://github.com/brillout/release-me doesn't use any Git nor CLI helper, but directly uses Node.js to exectue Git commands. The hard part is managing CLI commands, so if anything I'd say let's use a CLI helper instead of a Git helper. Directly using Node.js, as @brillout/release-me does, isn't that bad etiher. My suggestion: let's do it ourselves, unless you can find a neat CLI helper then I'm open for us to use it.

@snake-py
Copy link
Author

snake-py commented Sep 29, 2024

Alright, no objections from my side. All your comments are reasonable. Will work on this shortly.

Ok 👍

I just tried it, and it generates [this commit]
(e73b82c). I guess there is a bug?

Hmm, odd that you don't have the ejections folder 🤔 Maybe due to symlinks? I will look into it. I have not tested the thing in a monorepo set up.

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.

3 participants