jxnblk / jxnblk/mdx-deck

eject not working

Open
#511 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
11.5k
Forks
594
PR merge metrics
No merged PRs in 30d

Description

Using `mdx-deck eject` is currently not working as it should.

This is because the logic in [initit](https://www.npmjs.com/package/initit) does not match what the `eject` command expects. So either we send a PR to `initit` changing the logic (changing how it handles absolute paths) or we adapt `eject` to the actual logic/result/structure of `initit`.

Adapting `eject` to `initit` would basically require `eject` to have intricate knowledge of the logic inside `initit` and that doesn't feel right to me. I would rather try and get `initit` to alter the logic it uses and support absolute paths in the way I assume that `eject` expects.

## `initit` logic details

The logic inside `initit` is that it takes a `name` argument which it uses to create a folder in the current working directory. This makes sense if used as intended, you pass a string containing only the name of the directory you want to initialize in.

But `eject` doesn't pass a name, it passes the `cwd`, meaning an absolute path. Since `initit` expects a name, not an absolute path, it tries to resolve an absolute path based on the given name. This is where it gets tricked.

Given a "name":
```
cwd = /tmp/my-cwd
name = my-name
resolves: /tmp/my-cwd/my-name
creates: /tmp/my-cwd/my-name
```

Given an absolute path:
```
cwd = /tmp/my-cwd
name = /tmp/my-cwd
resolves = /tmp/my-cwd
creates = /tmp/my-cwd/my-cwd
```
As you see `initit` creates the given folder name based on the `basename` of the resolved path.

What I (and I assume also `eject`) expected was:
```
cwd = /tmp/my-cwd
name = /tmp/my-cwd
resolves = /tmp/my-cwd
creates (reuses really) = /tmp/my-cwd
```

I assume that `eject` expected `initit` to reuse the current working directory. If it relied on the current implementation described above, it nees to adjust its code to reflect the resulting directory structure from `initit`. Specifically this line `fs.moveSync(filename, path.join(cwd, 'decks'))` needs to change to something like `path.join(cwd, path.basename(cwd), `decks/${path.basename(filename)}`)` to match up with the resulting structure from the current `initit`.

I'm willing to send a PR both ways, would just like some guidance in what would be the accepted approach.

Contributor guide

Open the contributing guide

Research direction

Start with the `eject` command and inspect how it calls `initit`, especially the `fs.moveSync(filename, path.join(cwd, 'decks'))` path handling described in the issue. Reproduce the absolute-path behavior in `initit`, then determine whether the accepted fix belongs in `initit` or `eject`; done means eject places the deck files in the intended current working directory.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
cli
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.