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

Support for import followed by other statements in scripts piped to meteor shell #8833

Closed
wants to merge 2 commits into from

Conversation

atd
Copy link
Contributor

@atd atd commented Jun 22, 2017

Fixes #8823

@apollo-cla
Copy link

@atd: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@abernix
Copy link
Contributor

abernix commented Jun 22, 2017

Just noting that the test failure appears related to the change (and isn't just an unrelated fluke).

@benjamn benjamn changed the title Support for import in meteor shell scripts Support for import followed by other statements in scripts piped to meteor shell Jun 22, 2017
@benjamn benjamn added this to the Release 1.5.1 milestone Jun 22, 2017
// If repl didn't start, `require` and `module` are not visible
// in the vm context
repl.require = require;
repl.module = module;
Copy link
Contributor

@benjamn benjamn Jun 22, 2017

Choose a reason for hiding this comment

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

These are the require and module variables available globally in the shell-server.js module. Instead, I think we should be defining a module for the REPL using meteorInstall so that we can use the resulting require and module parameters, as we do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And of course now would be a great time to move the code that creates the REPL module into a helper function, since it's now used twice.

@benjamn
Copy link
Contributor

benjamn commented Jun 22, 2017

I canceled and rescheduled the most recent test run because it failed with a segmentation fault here, which is probably not your fault.

atd added 2 commits June 23, 2017 18:43
The prevented to run certain commands in scripts,
like `import { Foo } from './bar'`

See meteor#8823
@atd
Copy link
Contributor Author

atd commented Jun 23, 2017

I have extracted the function and ended up using the global object (it didn't work with repl and global is used in node's doc).

@benjamn
Copy link
Contributor

benjamn commented Jun 27, 2017

The test failure seems entirely unrelated, though it has happened more than once now. I'm going to merge this anyway, in order to get it into the first 1.5.1 release candidate. If that test keeps failing, we can keep debugging it on the release-1.5.1 branch.

@benjamn
Copy link
Contributor

benjamn commented Jun 27, 2017

Merged to devel as 2bcfb07 and 0b1a1ef.

@benjamn benjamn closed this Jun 27, 2017
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

Successfully merging this pull request may close these issues.

None yet

4 participants