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

Tech/rewrite plop in typescript #868

Merged
merged 22 commits into from
May 30, 2020
Merged

Conversation

alschmut
Copy link
Contributor

Tech/rewrite plop in typescript

As always with PRs from me, which I didn't communicate in advance, it's still everybody's responsibility to decide, if my proposed changes are relevant/useful and if they should be merged or not.

Description

I was wondering if it might be nice to split the plop generator into multiple files, each only responsible for one of the 4 major file changes (state, ui, util, redux) and if we could make use of typescript.

  • This PR splits the existing plop generator into multiple files, and some helper functions to decrease code duplication.
  • The plop module and the functions make use of Typescript

Issue to solve

Unfortunately plop doesn't run anymore since I changed the module declaration to make use of typescript as mentioned in the plop doc. When I undo using typescript, it says, imports are not allowed outside a module declaration.
=> Does anybody have an idea how we could solve this?

$ npm run g

> [email protected] g 
.../codecharta/visualization
> plop

.../codecharta/visualization/plopfile.ts:1
(function (exports, require, module, __filename, __dirname) { import { NodePlopAPI } from "plop"
                                                              ^^^^^^

SyntaxError: Unexpected token import
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:617:28)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at nodePlop (.../codecharta/visualization/node_modules/node-plop/lib/node-plop.js:268:5)

Definition of Done

A task/pull request will not be considered to be complete until all these items can be checked off.

  • All requirements mentioned in the issue are implemented
  • Does match the Code of Conduct and the Contribution file
  • Task has its own GitHub issue (something it is solving)
    • Issue number is included in the commit messages for traceability
  • Update the README.md with any changes/additions made
  • Update the CHANGELOG.md with any changes/additions made
  • Enough test coverage to ensure that uncovered changes do not break functionality
  • All tests pass
  • Descriptive pull request text, answering:
    • What problem/issue are you fixing?
    • What does this PR implement and how?
  • Assign your PR to someone for a code review
    • This person will be contacted first if a bug is introduced into master
  • Manual testing did not fail

@alschmut alschmut added the tech For technical stories without user impact (=refactoring stories). label Feb 23, 2020
@BridgeAR
Copy link
Member

import usage is opt-in. Depending on the Node.js version used you might either pass through a command line flag to use esm modules or you can set a property inside the package json to indicate that the files use esm modules.

@alschmut
Copy link
Contributor Author

Thanks @BridgeAR! I really googled and tried a lot, but I am not getting it to work. Do you mind having a very short try?

@alschmut alschmut mentioned this pull request Mar 5, 2020
11 tasks
@alschmut alschmut changed the title Tech/rewrite plop in typescript WIP: Tech/rewrite plop in typescript Mar 12, 2020
alschmut and others added 9 commits March 13, 2020 08:42
…n-typescript

# Conflicts:
#	visualization/plop/templates/redux-subreducer/actions.ts.hbs
#	visualization/plop/templates/redux-subreducer/reducer.spec.ts.hbs
#	visualization/plop/templates/redux-subreducer/reducer.ts.hbs
#	visualization/plop/templates/redux-subreducer/splitter.ts.hbs
#	visualization/plopfile.js
…n-typescript

# Conflicts:
#	visualization/plop/templates/redux/injector.spec.ts.hbs
@BridgeAR BridgeAR assigned alschmut and unassigned BridgeAR Apr 30, 2020
@NearW
Copy link
Contributor

NearW commented May 1, 2020

Upgrading plop and removing the broken compiler options in the script call fixed the issue.

However, I'm not a fan of compiling those files everytime you want to generate something.
There exists ts-node which can execute ts files as node does with node myFile.js without compiling them (at least they don't create compiled files visible to us).

However, adding ts-node to the project npm i -D ts-node and executing it, will result in the following error:

import promptFile from "inquirer-file";
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (internal/modules/cjs/loader.js:1072:16)

In order to fix this, you have to simply tell ts-node to compile it with another module. Using "commonjs" instead of "es2015" will make it work. Changing that in our tsconfig.json will break everything though. So we need to find a way to tell ts-node use another module when executing the plop call.
And there is a way to do that! --compiler-options see here
But that's just not working. I tried it for 1 hour. Can't get it to work.

@NearW
Copy link
Contributor

NearW commented May 1, 2020

plopjs/plop#192 (comment)

"if you're using a compiled language, you'll need to install and configure a loader supported by https://www.npmjs.com/package/interpret"

…n-typescript

# Conflicts:
#	visualization/package-lock.json
@BridgeAR
Copy link
Member

I'm not a fan of compiling those files everytime you want to generate something.
There exists ts-node which can execute ts files as node does with node myFile.js without compiling them (at least they don't create compiled files visible to us).

This is how TypeScript regularly works with Node.js. You first have to transpile it. It would of course also be possible to remove the file after execution. That way it would also "almost" be invisible without having to use ts-node. IMO using ts-node feels a bit heavy for what's required.

@alschmut
Copy link
Contributor Author

@NearW thanks for the upgrading fix!

I was now updating the script to move the compiled files into the dist folder. There they are almost invisible and will be ignored by git. I think this is a good enough solution for now and the advantage of having separated typescript files definitely outweighs the pain of keeping the compiled files.

@BridgeAR
Copy link
Member

@alschmut please rebase and is this still WIP or should this be ready?

@alschmut alschmut changed the title WIP: Tech/rewrite plop in typescript Tech/rewrite plop in typescript May 29, 2020
@alschmut
Copy link
Contributor Author

@BridgeAR in my opinion this is ready to merge.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

RSLGTM

@BridgeAR BridgeAR merged commit 6720db9 into master May 30, 2020
@BridgeAR BridgeAR deleted the tech/rewrite-plop-in-typescript branch May 30, 2020 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech For technical stories without user impact (=refactoring stories).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants