OpenTechStrategies / SimpleBook

A fork of the Collection Extension to print books from MediaWiki instances
1 stars 1 forks source link

Upgrade Compiling/Packaging to Typescript/Webpack #31

Closed hawkinsw closed 3 years ago

hawkinsw commented 3 years ago
  1. Slowly begin migrating code to Typescript from raw Javascript
  2. Instead of relying on built-in node module bundling, use webpack
  3. Add a yarn watch command that will rebuild mw2pdf when any TS file is edited. (see https://github.com/OpenTechStrategies/torque-devenv/issues/19)

These are prerequisites (in a very yak-shaving way) for adding unit testing to the code via Jest. See https://github.com/OpenTechStrategies/SimpleBook/issues/20.

hawkinsw commented 3 years ago

Yes!!!

Thank you for this work, it's excellent and moving to typescript is a great step forward for the project.

Thank you for the feedback!

Some small comments / questions but the only meaningful action items are:

1. Considering the use of `ts-loader` since it seems to be more supported.

Done!

2. Making sure the new entrypoint doesn't break the python call (check `app.py`).

I think that we are going to need to have a separate PR to update the app.py when this is merged. Are you opposed to that?

3. Spinning up an issue in devenv to have rebuilds-on-file-changes be automatic.

Will work this with you separately.

Thanks again for the review! Will

hawkinsw commented 3 years ago

Yes!!! Thank you for this work, it's excellent and moving to typescript is a great step forward for the project.

Thank you for the feedback!

Some small comments / questions but the only meaningful action items are:

1. Considering the use of `ts-loader` since it seems to be more supported.

Done!

2. Making sure the new entrypoint doesn't break the python call (check `app.py`).

I think that we are going to need to have a separate PR to update the app.py when this is merged. Are you opposed to that?

Let me know what you think here, @slifty.

Thanks! Will

3. Spinning up an issue in devenv to have rebuilds-on-file-changes be automatic.

Will work this with you separately.

Thanks again for the review! Will

slifty commented 3 years ago

@hawkinsw The only change I would still ask for is related to:

  1. Making sure the new entry point doesn't break the python call (check app.py).

You mentioned this might need to be a separate PR but if we do that then this PR would result in a broken build (temporarily, but still).

To be clear what I'm asking: basically that this block be changed: https://github.com/OpenTechStrategies/SimpleBook/blob/99bfcf66e30db9053eb675aac84830a9ba23920a/services/api/app.py#L20-L22

to something like:

    params = [
        "yarn",
        "--cwd", "mw2pdf"
        "main",
        ...
    ]

^ note that --cwd lets you run a yarn command from a different directory.

(where main is whatever you end up using to be the equivalent of start)

that said if I'm missing something please feel free to ignore this request and do it as a separate PR! This is lookin' good.

hawkinsw commented 3 years ago

@hawkinsw The only change I would still ask for is related to:

  1. Making sure the new entry point doesn't break the python call (check app.py).

You mentioned this might need to be a separate PR but if we do that then this PR would result in a broken build (temporarily, but still).

To be clear what I'm asking: basically that this block be changed:

https://github.com/OpenTechStrategies/SimpleBook/blob/99bfcf66e30db9053eb675aac84830a9ba23920a/services/api/app.py#L20-L22

to something like:

    params = [
        "yarn",
        "--cwd", "mw2pdf"
        "main",
        ...
    ]

^ note that --cwd lets you run a yarn command from a different directory.

(where main is whatever you end up using to be the equivalent of start)

that said if I'm missing something please feel free to ignore this request and do it as a separate PR! This is lookin' good.

After our interactive conversation earlier today I now realize what you meant. I have made the change in the most recent version of this PR. Let me know what you think!

slifty commented 3 years ago

Looks great! May wanna update the documented run command but fine either way!

Regardless no need for re-review; merge away when ready (though of course there are now branch conflicts :()

slifty commented 3 years ago

Just a note: I am in the process of revamping SimpleBook's installation in dev-env to use Ansible, and so we're waiting on that to get pushed before we can do the final test-and-merge of this PR. Stay tuned!

hawkinsw commented 3 years ago

Just a note: I am in the process of revamping SimpleBook's installation in dev-env to use Ansible, and so we're waiting on that to get pushed before we can do the final test-and-merge of this PR. Stay tuned!

And, as I said in "chat", take your time! I want to make sure that everything works so that I don't screw things up!

Will