Split core functionality from CLI #14

kachkaev · Apr 28, 2018

👋

I recently needed to use run-elm via require() rather than by spawning a sub-process and came up with a refactoring that involved splitting core functionality from CLI. At the moment this change is included into a fork called @kachkaev/run-elm, but I hope to deprecate that package once we update run-elm.

I have marked this PR as work-in-progress because it includes commits from #13, has some fork-specific commits and also lacks usage instructions for require() in README. The tests have been done though, all works!

One thing that bugs me is that I'm using compileSync instead of compile (they both replace compileToString and save one cycle of file write/read). The reason is in rtfeldman/node-elm-compiler#68 (async version calls process.exit() on error, so it can't be used when we require('run-elm')).

WDYT @jfairbank? ~~I'm happy to rebase and refactor after merging #13 and getting your general approval.~~

This seems like a good change to make. I'll wait for you to rebase before I review. Thanks!

jfairbank · May 8, 2018

This seems like a good change to make. I'll wait for you to rebase before I review. Thanks!

Despite that all tests pass, I'm still not sure this PR is worth merging as is. The issue that I see is that we're calling node-elm-compiler synchronously, because otherwise we'll get early exit due to rtfeldman/node-elm-compiler#68. Therefore, the main Node.js thread will get blocked and performance will suffer (this only applies when Node API is used). There is no regression in CLI use, just a non-optimality in the new execution mode.

What are your thoughts on this @jfairbank? How is the PR in general?

kachkaev · May 8, 2018

Despite that all tests pass, I'm still not sure this PR is worth merging as is. The issue that I see is that we're calling node-elm-compiler synchronously, because otherwise we'll get early exit due to rtfeldman/node-elm-compiler#68. Therefore, the main Node.js thread will get blocked and performance will suffer (this only applies when Node API is used). There is no regression in CLI use, just a non-optimality in the new execution mode.

What are your thoughts on this @jfairbank? How is the PR in general?

I also noticed today that Elm's Debug.log is being incorrectly reported in the API mode. Will add a fix to this soon, please don't merge.

kachkaev · May 14, 2018

I also noticed today that Elm's Debug.log is being incorrectly reported in the API mode. Will add a fix to this soon, please don't merge.

I also managed to fix a few things so that they work on Windows:
https://ci.appveyor.com/project/kachkaev/run-elm/build/27

Things to note:

stderr / stdout never contain \r (even when it's Windows) to make sure there are no unwanted parsing issues in the downstream projects. If someone asks for a nice-looking output for cmd, we can look into adding a CLI option for that.
I had to rename Aux.elm into Helper.elm in one of the tests, because you can't use AUX for file names on Windows 😱 This was giving a pretty strange exception.

@jfairbank feel free to integrate AppVeyor to the repo just as I did in my fork.

One last thing I'd like to look into is to make a call to elm compiler async despite rtfeldman/node-elm-compiler#68.

kachkaev · May 15, 2018

I also managed to fix a few things so that they work on Windows:
https://ci.appveyor.com/project/kachkaev/run-elm/build/27

Things to note:

stderr / stdout never contain \r (even when it's Windows) to make sure there are no unwanted parsing issues in the downstream projects. If someone asks for a nice-looking output for cmd, we can look into adding a CLI option for that.
I had to rename Aux.elm into Helper.elm in one of the tests, because you can't use AUX for file names on Windows 😱 This was giving a pretty strange exception.

@jfairbank feel free to integrate AppVeyor to the repo just as I did in my fork.

One last thing I'd like to look into is to make a call to elm compiler async despite rtfeldman/node-elm-compiler#68.

kachkaev · May 15, 2018

kachkaev reviewed May 15, 2018

View changes

kachkaev · May 15, 2018

package.json

  },
  "scripts": {
-    "test:clean": "cd test/integration && git clean -d -f -X",
+    "clean:test": "git clean -d -f -X test/integration",


&& are not allowed in npm scripts on Windows; also renamed test:clean into clean:test for a better clarity

kachkaev · May 15, 2018

kachkaev reviewed May 15, 2018

View changes

kachkaev · May 15, 2018

package.json

    "lint": "eslint src test --ignore-pattern elm-stuff",
+    "prebuild": "rimraf lib",
+    "build": "babel src --out-dir lib",
+    "postbuild": "cpx \"src/*.elm.template\" lib",


&& are not allowed in npm scripts on Windows, so I had to split command into three

kachkaev · May 15, 2018

kachkaev reviewed May 15, 2018

View changes

kachkaev · May 15, 2018

src/index.js

@@ -70,10 +37,10 @@ import sh from 'shelljs';

    // ensure --output-name is specified adequately
    if (!outputName.match(/^[a-z_]\w*$/)) {
-      throw new Error(`Provided --output-name \`${outputName}\` is not a valid constant or function name in elm.`);
+      throw new Error(`Provided output name \`${outputName}\` is not a valid constant or function name in elm.`);


not using --output-name to avoid confusion in non-CLI usage. Same applies to other args

kachkaev · May 15, 2018

kachkaev reviewed May 15, 2018

View changes

kachkaev · May 15, 2018

src/index.js

+    await Promise.all([
+      close(elmCompileStdoutFd).catch(() => {}),
+      close(elmCompileStderrFd).catch(() => {}),
+    ]);


Had to do this to support Windows, otherwise the files were left unclosed and could not be deleted

The PR should be ready for your review @jfairbank 🎉 Calls to elm-compile are async now and both Travis and AppVeyor#28 (Windows) pass!

kachkaev · May 15, 2018

The PR should be ready for your review @jfairbank 🎉 Calls to elm-compile are async now and both Travis and AppVeyor#28 (Windows) pass!

ping @jfairbank 🏓 🙂

kachkaev · May 21, 2018

ping @jfairbank 🏓 🙂

Hey, @kachkaev. I've been at a conference this week and am traveling the rest of the week, so I won't be able to check this PR more thoroughly until next week. Sorry for the delay, but I didn't want to keep you in the dark at least.

jfairbank · May 23, 2018

Hey, @kachkaev. I've been at a conference this week and am traveling the rest of the week, so I won't be able to check this PR more thoroughly until next week. Sorry for the delay, but I didn't want to keep you in the dark at least.

@jfairbank when about could you have a look at this? No rush, just curious.

kachkaev · Jun 7, 2018

@jfairbank when about could you have a look at this? No rush, just curious.

Sorry again; I've just been super busy with other priorities. I finally looked this over, and it seems reasonable. I'll get a release out later tonight. Thanks!

jfairbank · Jun 8, 2018

Sorry again; I've just been super busy with other priorities. I finally looked this over, and it seems reasonable. I'll get a release out later tonight. Thanks!

v2.3.0 is now out!

jfairbank · Jun 8, 2018

v2.3.0 is now out!

kachkaev referenced this pull request May 8, 2018
Merged
Add full stops to some error messages for consistency #15

kachkaev changed the title from [WIP] Split core functionality from CLI to Split core functionality from CLI May 8, 2018

kachkaev referenced this pull request in rtfeldman/node-elm-compiler May 15, 2018

Throw instead of warning.
Fixes #68

fbb37d4

Use async version of compile() with a workaround

Verified

This commit was signed with a verified signature.

kachkaev Alexander Kachkaev

GPG key ID: DEFE41F826386E2E Learn about signing commits

565c766

jfairbank merged commit 8fa0acc into jfairbank:master Jun 8, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jfairbank/run-elm

Split core functionality from CLI #14

kachkaev referenced this pull request May 8, 2018

Add full stops to some error messages for consistency #15

This comment has been minimized.

jfairbank commented May 8, 2018

kachkaev added some commits Apr 26, 2018

This comment has been minimized.

kachkaev commented May 8, 2018 •

edited

Edited 5 times

kachkaev edited May 8, 2018 (most recent)

kachkaev edited May 8, 2018

kachkaev edited May 8, 2018

kachkaev edited May 8, 2018

kachkaev edited May 8, 2018

kachkaev changed the title from [WIP] Split core functionality from CLI to Split core functionality from CLI May 8, 2018

kachkaev added some commits May 8, 2018

This comment has been minimized.

kachkaev commented May 14, 2018

kachkaev added some commits May 14, 2018

This comment has been minimized.

kachkaev commented May 15, 2018 •

edited

Edited 1 time

kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018

View changes

This comment has been minimized.

kachkaev reviewed May 15, 2018

View changes

This comment has been minimized.

kachkaev reviewed May 15, 2018

View changes

This comment has been minimized.

kachkaev reviewed May 15, 2018

View changes

This comment has been minimized.

kachkaev referenced this pull request in rtfeldman/node-elm-compiler May 15, 2018

This comment has been minimized.

kachkaev commented May 15, 2018 •

edited

Edited 1 time

kachkaev edited May 15, 2018 (most recent)

This comment has been minimized.

kachkaev commented May 21, 2018 •

edited

Edited 1 time

kachkaev edited May 21, 2018 (most recent)

This comment has been minimized.

jfairbank commented May 23, 2018

This comment has been minimized.

kachkaev commented Jun 7, 2018

jfairbank merged commit `8fa0acc` into jfairbank:master Jun 8, 2018
1 check passed

1 check passed

This comment has been minimized.

jfairbank commented Jun 8, 2018

This comment has been minimized.

jfairbank commented Jun 8, 2018

jfairbank/run-elm

Join GitHub today

Split core functionality from CLI #14

Conversation

kachkaev referenced this pull request May 8, 2018

This comment has been minimized.

jfairbank commented May 8, 2018

kachkaev added some commits Apr 26, 2018

This comment has been minimized.

kachkaev commented May 8, 2018 • edited Edited 5 times kachkaev edited May 8, 2018 (most recent) kachkaev edited May 8, 2018 kachkaev edited May 8, 2018 kachkaev edited May 8, 2018 kachkaev edited May 8, 2018

kachkaev changed the title from [WIP] Split core functionality from CLI to Split core functionality from CLI May 8, 2018

kachkaev added some commits May 8, 2018

This comment has been minimized.

kachkaev commented May 14, 2018

kachkaev added some commits May 14, 2018

This comment has been minimized.

kachkaev commented May 15, 2018 • edited Edited 1 time kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018 View changes

This comment has been minimized.

kachkaev May 15, 2018 • edited Edited 1 time kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018 View changes

This comment has been minimized.

kachkaev reviewed May 15, 2018 View changes

This comment has been minimized.

kachkaev May 15, 2018 • edited Edited 1 time kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018 View changes

This comment has been minimized.

kachkaev referenced this pull request in rtfeldman/node-elm-compiler May 15, 2018

This comment has been minimized.

kachkaev commented May 15, 2018 • edited Edited 1 time kachkaev edited May 15, 2018 (most recent)

This comment has been minimized.

kachkaev commented May 21, 2018 • edited Edited 1 time kachkaev edited May 21, 2018 (most recent)

This comment has been minimized.

jfairbank commented May 23, 2018

This comment has been minimized.

kachkaev commented Jun 7, 2018

Hide details View details jfairbank merged commit 8fa0acc into jfairbank:master Jun 8, 2018 1 check passed

1 check passed

This comment has been minimized.

jfairbank commented Jun 8, 2018

This comment has been minimized.

jfairbank commented Jun 8, 2018

kachkaev commented May 8, 2018 •

edited

Edited 5 times

kachkaev edited May 8, 2018 (most recent)

kachkaev edited May 8, 2018

kachkaev edited May 8, 2018

kachkaev edited May 8, 2018

kachkaev edited May 8, 2018

kachkaev commented May 15, 2018 •

edited

Edited 1 time

kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018

View changes

kachkaev May 15, 2018 •

edited

Edited 1 time

kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018

View changes

kachkaev reviewed May 15, 2018

View changes

kachkaev May 15, 2018 •

edited

Edited 1 time

kachkaev edited May 15, 2018 (most recent)

kachkaev reviewed May 15, 2018

View changes

kachkaev commented May 15, 2018 •

edited

Edited 1 time

kachkaev edited May 15, 2018 (most recent)

kachkaev commented May 21, 2018 •

edited

Edited 1 time

kachkaev edited May 21, 2018 (most recent)

jfairbank merged commit `8fa0acc` into jfairbank:master Jun 8, 2018
1 check passed