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

Console logs from imported modules in AWS template file #18

Open
rafhofman opened this issue Nov 20, 2018 · 12 comments
Open

Console logs from imported modules in AWS template file #18

rafhofman opened this issue Nov 20, 2018 · 12 comments

Comments

@rafhofman
Copy link

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

  1. Import a module to cloudform typescript template
  2. Use cloudform to generate aws template
  3. console logs from imported files are included in aws template

Expected behavior

Console logs of imported modules should be printed to console, instead of aws template

Minimal reproduction of the problem with instructions

Create a module which has one function that console.log something. Import it to cloudform template. Generate aws template - your console log will be in the first line

What is the motivation / use case for changing the behavior?

It is usefull to import some types/interfaces from modules - this issue disallows such option

Environment


Cloudform version: 3.3.0

 
For Tooling issues:
- Node version: 10.13.0
- Platform:  Mac

Others:
- Yarn 1.10.1
@NOtherDev
Copy link
Collaborator

Thanks @RafalBDS.

I'm not sure I should do anything about it in the way cloudform currently operates. Note that outputting the template via the stdout stream is the core API part of cloudform. If you decide to put something more into stdout from your code, then yes, it will also be included in the output, by definition.

Option I consider is to play with process's streams and ensure your console.logs don't actually go to stdout. But it would be probably more surprising than what we have right now.

I'd suggest using console.warn or its friends - it outputs to stderr stream instead.
Let me know and reopen the issue if it doesn't solve your use case.

@NOtherDev
Copy link
Collaborator

Also, I'm not sure what do you mean about this part of the issue:

It is usefull to import some types/interfaces from modules - this issue disallows such option

Could you please clarify?

@rafhofman
Copy link
Author

@NOtherDev
Nothing special in particular - I had an issue when importing interface definition from the module that was also consol.loging by default. Thank you for your explanation - this indeed sounds reasonable. Will look into the code and maybe try to help with some PR - at least by updating README and warning about such behavior :)

@miensol
Copy link
Member

miensol commented Dec 4, 2018

I'm actually quite surprised this issue turned out to not be an issue :)

Given the following code

import cloudform, {Fn, Refs, EC2, StringParameter, ResourceTag} from "cloudform"

console.log("Hello");

export default cloudform({
   ...
})

I wouldn't expect "Hello" to be printed out. I'm explicitly exporting an object created by cloudform and running it through cloudform CLI. I would expect cloudform CLI to understand that only the exported part is my template, and only that that should be turned into CloudForamation input...

Note that outputting the template via the stdout stream is the core API part of cloudform.

@NOtherDev could you please elaborate why this is the case?

@NOtherDev
Copy link
Collaborator

All I said is that cloudform CLI does not create a template file for you. It outputs it to stdout and it's your code that writes it to the file, normally via stream redirection. So whatever you output to the stream from your code will land there.

We might consider creating output file by cloudform itself as an option (or as the way to go by default in next major). I can't see how can we overcome this in any other sane way. Do you?

@miensol
Copy link
Member

miensol commented Dec 6, 2018

What I was thinking about is to suppress any stout/stderr output printed by executing the template file.

IMHO the readme hints pretty clearly that one should use export default cloudform(...). The fact that there's something exported from the file in turn suggests that it's only the exported object which matters.

Had the export not be required it would be more easy to understand that cloudform(...) function invocation inside the template prints to stdout and thus that one should be careful about other places that might write something.

@NOtherDev
Copy link
Collaborator

OK, but on the other hand, if you explicitly do console.log, isn't it the idea to have it on stdout? I find suppressing stdout potentially much more surprising than the fact that ALL stdout goes into the output that you handle using standard commands.

@miensol
Copy link
Member

miensol commented Dec 7, 2018

I agree that suppressing it completely could be surprising as well 🤔

How about redirecting all writes performed during template loading to stderr?

This could be as follows:

ts-node -e "const print = process.stdout.write.bind(process.stdout); process.stdout.write = process.stderr.write.bind(process.stderr); import template from '/Users/piotr/dev/typescript-sample/modA/template'; print(template + '\n')"

Then the standard cloudform ./sample.ts > output.json would still work as expected.

@NOtherDev
Copy link
Collaborator

NOtherDev commented Dec 7, 2018

It's still the same type of confusion. Why stderr when you wrote to stdout?
I suggested Rafał to use console.warn or its friends as it outputs to stderr stream instead.
By now the only solution that seems good for me is to change the API and handle file write by cloudform itself: cloudform ./sample.ts output.json instead of cloudform ./sample.ts > output.json. It's a breaking change for rather non-critical issue, though...

@miensol
Copy link
Member

miensol commented Dec 7, 2018

I agree that this is more easy to follow cloudform ./sample.ts output.json perhaps even better would be cloudform --template=./sample.ts --output=output.json.

Why stderr when you wrote to stdout?

Because stdout, as documented, is what cloudform uses. The example suggest that you need to do export default cloudform to generate the template. This is why neither Rafał nor me expected console.log would interfere. I fully understand though why it's the case. Perhaps there would be no confusion if the example used an explicit console.log(cloudform(...)). This doesn't look nice though.

Why is cloudform ./sample.ts output.json a breaking change?

@NOtherDev
Copy link
Collaborator

Why is cloudform ./sample.ts output.json a breaking change?

Valid question. It is not. Just let me find some spare time, we'll have it in v3.6.

@NOtherDev NOtherDev reopened this Dec 10, 2018
@fhewitt
Copy link
Contributor

fhewitt commented Dec 10, 2018

Writing to stdout can be helpful when you try to pipe CLI commands, so I believe that the current approach should stay the default.

Given the miensol example, the easy solution is to use the SDK approach instead of the CLI, and write the file manually in the code. In which case, you definitely expect to see your "console.log('Generating template')" and "console.log('Saving template') in stdout.

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

No branches or pull requests

4 participants