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

[ALOY-1722] Make alloy work as required module #956

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brentonhouse
Copy link
Contributor

No description provided.

@build
Copy link

build commented Mar 6, 2020

Warnings
⚠️

Please ensure to add a changelog entry for your changes. Edit the CHANGELOG.md file and add your change under the Unreleased items header

⚠️

🔍 Can't find junit reports at ./TEST-*.xml, skipping generating JUnit Report.

Generated by 🚫 dangerJS against d6437ab

@janvennemann
Copy link
Contributor

Now the Alloy require still has an indirect dependency on commander as the constructor expects a configured and parsed program. That doesn't feel very intuitive.

I guess you are just trying to run Alloy compile on a project? I'd say a simple compile function that accepts the project path and platform (and whatever else it may need) would be a more sophisticated API, don't you think. So something like this:

const { compile } = require('alloy');
compile({
  project: '/path/to/my/app'
  platform: 'ios'
});

Also, we should think about what part's of the Alloy CLI actually make sense to expose as a proper node API. I know this is just a quick and dirty solution, but as you see there are more things to consider.

@brentonhouse
Copy link
Contributor Author

@janvennemann - correct. I think the parameters and other things should be updated to that the CLI takes its parameters and correctly passes those to the alloy file. Unfortunately, when this was built, it was written to expect the "program" object from commander to be passed around to files all over in the library. I'd say this is a good first step and can be merged now as it is better than the existing solution and doesn't break anything.

@ewanharris
Copy link
Contributor

ewanharris commented Mar 9, 2020

I would prefer for us to deal with this as planned in Alloy 2 alongside #952 and related tickets. I don't think we should make the commander definition the interface here but instead as Jan suggested provide a simpler interface that abstracts that implementation detail (commander) away.

Rather than try and rush to provide a working solution on 1.x, let's work together (and communicate before we land a PR) to find something that fits the goals we all want from 2.x.

@janvennemann
Copy link
Contributor

In the meantime, if you really need to require Alloy commands right now, have you tried requiring the command you need directly? Wouldn't that have the same effect?

const compile = require('alloy/Alloy/commands/compile');
const program = {};
compile(program);

You loose some sanity checks regarding banner/help display, but i assume you know what you are doing so you can make sure to pass a properly configured program to the command.

@brentonhouse brentonhouse self-assigned this Oct 3, 2020
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.

4 participants