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

add support for chakracore #1828

Closed
wants to merge 1 commit into from

Conversation

patrickkettner
Copy link

👋

cc @mathias

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR - this certainly is a solid amount of work.

Please see my below comment; it might have been prudent to discuss the implementation before writing code, since I'd hate to see virtually all of this effort go to waste - but to add this kind of complexity to nvm requires careful consideration and planning.

@@ -369,6 +373,8 @@ nvm_version_dir() {
nvm_echo "${NVM_DIR}/versions/node"
elif [ "_${NVM_WHICH_DIR}" = "_iojs" ]; then
nvm_echo "${NVM_DIR}/versions/io.js"
elif [ "_${NVM_WHICH_DIR}" = "_chakra" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

chakra isn't the same category as "node" or "io.js" - it's an engine, not a platform. node and io.js are implicitly v8, so I'd want an entirely new way to explicitly specify the engine that you're using with the particular node version (and alias "v8" to the current implicit default)

The engine here is more akin to the way I've been planning to support nightlies and release candidates; and I'll want to make sure that engine support both works well with those, and doesn't force a specific implementation path for them.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, what syntax do you imagine then?

Copy link
Member

Choose a reason for hiding this comment

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

for RCs, I'd initially begun with iojs:rc-v1.2.3 and node:rc-v4.5.6 (which would be the same here; you could do node:chakra, for example).

However, my in-progress branch also does some significant refactoring of the internals of nvm to be able to achieve this cleanly.

@ljharb ljharb added feature requests I want a new feature in nvm! needs followup We need some info or action from whoever filed this issue/PR. labels Jun 7, 2018
@ljharb
Copy link
Member

ljharb commented Jun 7, 2018

See #1067 as well for further discussion.

@patrickkettner
Copy link
Author

patrickkettner commented Jun 7, 2018 via email

@ljharb
Copy link
Member

ljharb commented Jun 7, 2018

A flag would be nice; but that wouldn't allow specifying things in aliases or in .nvmrc files (that's why lts/* and lts/argon etc are a thing). In other words, the engine needs to be a part of the version string.

@ljharb
Copy link
Member

ljharb commented Jun 7, 2018

Specifically, the use case I personally have in mind is being able to run tests in CI against v8-node and chakra-node, which means i need to be able to specify the engine in travis.yml's node_js key.

@patrickkettner
Copy link
Author

got it, thanks.

so to be clear, something like

node:chakracore-v10.1.0 for stable, and node:chakracore-v10.0.0-rc.0 for rc/nightly/whatever?

@ljharb
Copy link
Member

ljharb commented Jun 9, 2018

that seems reasonable, yes. if you want to update this PR with that in mind, i'll try to octopus merge it with my RC branch locally - we might be able to get both in at the same time, without conflicting.

@patrickkettner
Copy link
Author

patrickkettner commented Jun 9, 2018

you haven't pushed any of your branch, right @ljharb ?

@ljharb
Copy link
Member

ljharb commented Jun 10, 2018

It's not really in a state for that quite yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! needs followup We need some info or action from whoever filed this issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants