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

Cannot use "import" in `meteor shell` via STDIN #8823

Closed
atd opened this Issue Jun 20, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@atd
Contributor

atd commented Jun 20, 2017

While using import in a normal meteor shell works

$ meteor shell

Welcome to the server-side interactive shell!

Tab completion is enabled for global variables.

Type .reload to restart the server and the shell.
Type .exit to disconnect from the server and leave the shell.
Type .help for additional help.

> import { Success } from "./imports/collections";
> Success
'success!'
> Shell exiting...

The same command using STDIN throws a SyntaxError

$ echo 'import { Success } from "./imports/collections"' | meteor shell
SyntaxError: Unexpected token, expected ( (1:8)

Seen in both meteor 1.4.4.3 and 1.5

@atd

This comment has been minimized.

Contributor

atd commented Jun 20, 2017

The issue can be reproduced using https://github.com/atd/meteor-issue-8823

@atd

This comment has been minimized.

Contributor

atd commented Jun 20, 2017

So I guess the cause of the issue is this line

"(" + options.evaluateAndExit.command + ")",

I think I found the solution to the comma syntax mistery

The funny thing is that you can build a "meteor shell command injection" and bypass the parenthesis restriction:

$ echo '""); import { Success } from "./imports/collections"; (Success' | meteor shell
"success!"

So why the parenthesis wrapping when just sending multiple commands seems to work?

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 20, 2017

If I recall correctly, the reason was to make sure the code is treated as an expression, so that it evaluates to a value that can be echoed back.

Would you mind playing with the shell-server.js implementation to see what happens if you don't force the expression with parentheses? We'd be happy to consider a pull request!

@atd

This comment has been minimized.

Contributor

atd commented Jun 21, 2017

It seems things work like before: the output shows the value of the last expression

$ echo 'var a = 1' | meteor shell                                                    
undefined
$ echo 'var a = 1; a + 1' | meteor shell                                             
2
$ echo 'console.log("hi"); var a = 1; a + 1' | meteor shell                        
2

(the server outputs I20170621-12:19:27.953(2)? hi)

The variables are saved across executions:

$ echo 'a + 1' | meteor shell                                             
2

There is an additional problem through: require and module are missing. They are added to the repl context here, but they seem not to be present when the vm script is run, (through they should be present in the context?)

$ echo 'import { Success } from "./imports/collections"' | meteor shell              
ReferenceError: module is not defined

However, they present once the import has been run in a repl

$ meteor shell 

Welcome to the server-side interactive shell!

Tab completion is enabled for global variables.

Type .reload to restart the server and the shell.
Type .exit to disconnect from the server and leave the shell.
Type .help for additional help.

> import { Success } from './imports/collections';
> Success
'success!'
> Shell exiting...

$ echo 'import { Ok } from "./imports/collections"; Ok' | meteor shell
"ok"

I'd be happy to prepare a pull request.

@atd

This comment has been minimized.

Contributor

atd commented Jun 21, 2017

An explanation of the module missing issue

nodejs/node-v0.x-archive#9211 (comment)

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 21, 2017

@atd Thanks for all the digging! I would just point out that we're already implementing our own REPL server, so we have a lot of control over what variables are exposed to it, whereas I think that issue is concerning the default Node REPL. In other words, we should be able to make this work. Please do submit a PR for any changes you'd like to see, even if it's still a work in progress, whenever you get a chance.

atd added a commit to atd/meteor that referenced this issue Jun 22, 2017

Remove parentheses wrapper of evaluateAndExit commmand
The prevented to run certain commands in scripts,
like `import { Foo } from './bar'`

See meteor#8823

atd added a commit to atd/meteor that referenced this issue Jun 22, 2017

Remove parentheses wrapper of evaluateAndExit commmand
The prevented to run certain commands in scripts,
like `import { Foo } from './bar'`

See meteor#8823
@atd

This comment has been minimized.

Contributor

atd commented Jun 22, 2017

The issue is that repl is not started when running a script, this line is not reached and therefore require and module aren't added to the context

I have prepare a quick hack to make them available: #8833
Note that require and module end up being added to this (or repl) because it is the same object used in repl.context. In any case, I am open to comments and improvements on the approach.

Finally, I am wondering why you don't just pipe the streams in https://github.com/meteor/meteor/blob/devel/tools/shell-client.js I think it would make meteor shell more powerful

@abernix

This comment has been minimized.

Member

abernix commented Jun 22, 2017

@atd Thanks for looking into this! I think your links are missing the line number anchors (e.g. #L15) so I didn't immediately see what you were referring to.

It's worth pointing out that a lot of the REPL implementation was done before much of the repl functionality in Node.js was fully exposed. Some parts (for example, this code catching to get the previously not-exported RecoverableError error class) are simply not necessary anymore.

There are new examples in the docs (linked above) so maybe worth re-evaluating those.

@atd

This comment has been minimized.

Contributor

atd commented Jun 22, 2017

Ok, so tests exposed the reason of the parentheses:

$ echo "{server:Meteor.isServer}" | /home/atd/dev/meteor/meteor shell
true

Sorry @abernix which part needs clarification? The reason of why require and module need to be added to context, the streams pipe question, or both?

@abernix

This comment has been minimized.

Member

abernix commented Jun 22, 2017

@atd I was just pointing out that some of the context you were referring to viashe'll-client.js links were pointing to the whole file, not specific lines (you referred to "here" as if you were pointing to a specific line). So yes, all of the above, but specificaly I think the stream question. ;)

@benjamn benjamn added this to the Release 1.5.1 milestone Jun 22, 2017

atd added a commit to atd/meteor that referenced this issue Jun 23, 2017

Remove parentheses wrapper of evaluateAndExit commmand
The prevented to run certain commands in scripts,
like `import { Foo } from './bar'`

See meteor#8823
@atd

This comment has been minimized.

Contributor

atd commented Jun 23, 2017

Ok, so I was wondering why shell-client.js handles the script case in a different way by just sending a evaluateAndExit command and it just doesn't pipe the process.stdin like in the tty case.

I think that it would allow process piping at the OS shell level and do more powerful things. Besides using the same code in shell-server.js

benjamn added a commit that referenced this issue Jun 27, 2017

Remove parentheses wrapper of evaluateAndExit commmand
The prevented to run certain commands in scripts,
like `import { Foo } from './bar'`

See #8823
@benjamn

This comment has been minimized.

Member

benjamn commented Jul 12, 2017

Should be fixed in shell-server@0.2.4, officially released with Meteor 1.5.1.

@benjamn benjamn closed this Jul 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment