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

ESM and simplified config #90

Merged
merged 8 commits into from
Jul 15, 2024
Merged

ESM and simplified config #90

merged 8 commits into from
Jul 15, 2024

Conversation

jannis-baum
Copy link
Owner

@jannis-baum jannis-baum commented Jul 14, 2024

This PR makes two bigger changes:

  1. It changes Vivify to be an ES Module and no longer CommonJS: Seems to be a trend that people are moving to ESM. Using ESM-only dependencies from CommonJS is a pain. We had a whole very ugly file just to use the ANSI-parser before because it was ESM-only and for this PR I had to adopt a second ESM-only (open) dependency so I decided to make the switch.
    • the build setup now uses tsc to compile to JavaScript before bundling with webpack because I couldn't get it to work in one go anymore (though it also seems more understandable now)
    • imports of local files now have a .js suffix
    • the importing of non-typed markdown-it modules got a bit more annoying
    • some other things got simplified quite a bit, most notably ESM dependencies
  2. The config is now handled entirely by vivify-server, making it a lot more clear and manageable, and viv gets simplified a lot in that we no longer need jq or an open/xdg-open-command.

@jannis-baum
Copy link
Owner Author

@tuurep I would appreciate if you could review this as well after #72 is done! But no worries if it's too much work. Just let me know :)

@jannis-baum jannis-baum mentioned this pull request Jul 14, 2024
7 tasks
@tuurep
Copy link
Collaborator

tuurep commented Jul 14, 2024

I don't know much about CommonJS and/or ESM but I made sure stuff builds - 👍

Then tested my configs still apply - 👍

But when running I notice one big problem:

I can no longer open more than one viewer at a time, example:

$ viv README.md            # Opens viewer on the browser
$ viv CONTRIBUTING.md      # Does nothing!
$ killall vivify-server
$ viv CONTRIBUTING.md      # Now I can open this, but nothing else at the same time

@jannis-baum
Copy link
Owner Author

Awesome, thanks!

But when running I notice one big problem:

I can no longer open more than one viewer at a time, example:

This is strange👀 I just tried it again on mine and can't reproduce it unfortunately, for me it works. Could you try debugging it? Suggestions:

  1. Ensure you have the current viv and not the old one
  2. Try calling vivify-server directly. viv is just a wrapper now to start and nohup the vivify-server, so you could do vivify-server README.md (this should start the server and open the README, keeping the terminal blocked because the server is running there), and then vivify-server CONTRIBUTING.md (this should just open the CONTRIBUTING.md and then exit)
  3. If 2. has the same behavior, check/print-debug what happens in the last 24 lines of src/app.ts; these are responsible for the opening and should be at fault then.
    If 2. works, then viv is at fault and we have to look into it there.

@tuurep
Copy link
Collaborator

tuurep commented Jul 14, 2024

I'll suggest this on a hunch before starting to debug properly:

Could it be an asynchronity thing?

get(`${address}/health`, () => {
    // server is already running
    openArgs();
    process.exit(0);
}).on('error', () => {
    // server is not running so we start it
    server.listen(config.port, async () => {
        console.log(`App is listening on port ${config.port}!`);
        openArgs();
    });
});

Something executing out of order and stuff like that? I always make these mistakes when I deal with these.

@tuurep
Copy link
Collaborator

tuurep commented Jul 14, 2024

Ah, my fork doesn't have the Actions the same way this repo does. Lets see how I'd get the build artifact (just trying to add some debug console logs).

@tuurep
Copy link
Collaborator

tuurep commented Jul 14, 2024

Although hold on. Now is an appropriate time to check the nvm thing here

Update:

Yes, I can build using nvm. Don't worry about the build artifact 😄

@tuurep
Copy link
Collaborator

tuurep commented Jul 14, 2024

So I added a test console log in the // server is already running block and it always logs, but appears as if process.exit(0) is done before openArgs()

@tuurep
Copy link
Collaborator

tuurep commented Jul 14, 2024

More logging reveals it reaches

open(url);

in openArgs()

but executes process.exit(0); before open(url); which looks like it's async: https://www.npmjs.com/package/open

src/app.ts Show resolved Hide resolved
jannis-baum added a commit that referenced this pull request Jul 15, 2024
jannis-baum added a commit that referenced this pull request Jul 15, 2024
@tuurep
Copy link
Collaborator

tuurep commented Jul 15, 2024

Ok! Are we ready to merge?

@jannis-baum jannis-baum merged commit 83d29ed into main Jul 15, 2024
4 checks passed
@jannis-baum jannis-baum deleted the esm-and-simplified-config branch July 15, 2024 15:44
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.

2 participants