Skip to content
This repository has been archived by the owner on May 11, 2018. It is now read-only.

[WIP] Add node's "engines" option #114

Closed
wants to merge 33 commits into from
Closed

Conversation

yavorsky
Copy link
Member

@yavorsky yavorsky commented Jan 1, 2017

Motivation:

In many cases, targets' node option is matching engines from project's package.json. Devs have to duplicate this value. In cases with ranges like > 0.12 they must determine next node.js version and specify it in env-preset's targets.
"current" or true option is not fully covering preset's mission. We could have last node, but maintain older versions.

So for ex:
babelrc

{
  presets: [
    [ "env", {
      "targets": {
        "node": "engines",
        "browsers": [ "chrome 54" ]
      }
    }]
  ]
}

package.json

  "engines" : { "node" : ">=0.12" }

will use targets:

{
  node: 0.12,
  chrome: 54
}

@yavorsky
Copy link
Member Author

yavorsky commented Jan 2, 2017

Also, we could use filterSatisfiedVersions, getVersionsFromList and desemverify from there to parse versions including ranges if target version is not a number.

targets {
  "node": ">=5.0",
  "chrome": "^1.1"
}

#107
#106

@existentialism
Copy link
Member

@yavorsky I was thinking the same thing, in fact had a rough branch using semver dep as well!

On an aside, should we move adding the yarn lock file to a separate PR (similar to #59) with CI runs with and without it?

test/index.js Outdated
});
});

describe("getLowestFromSemverValue", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit, but may want a test * returns null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Need to cover * as well. Thanks!

@yavorsky
Copy link
Member Author

yavorsky commented Jan 2, 2017

Oh, lock.file, always forget 😤. I think we must add it to .gitignore.

@hzoo
Copy link
Member

hzoo commented Jan 4, 2017

cc @sindresorhus since you expressed this idea (although as more of a default) in babel/babel#3476 (comment)

@yavorsky
Copy link
Member Author

yavorsky commented Jan 6, 2017

Update:

  • envEngines support:
    • Getting value from: BABEL_ENV -> NODE_ENV -> 'development'.
    • If no devEngines found with development environment - trying to access engines.
  • Remove yarn.lock, add it to .gitignore.
  • Update tests:
    • Cover '*' engines value
    • Cover engines value with wrong format.
    • Cover development/production environments.

package.json Outdated
@@ -7,6 +7,12 @@
"license": "MIT",
"repository": "https://github.com/babel/babel-preset-env",
"main": "lib/index.js",
"engines" : {
"node" : ">=0.12.0"
Copy link
Member

Choose a reason for hiding this comment

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

0.10 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, updated

package.json Outdated
"node" : ">=0.12.0"
},
"devEngines" : {
"node" : ">= 4.x"
Copy link
Member

Choose a reason for hiding this comment

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

we can do 6 here (I dont remember)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@sindresorhus
Copy link
Member

sindresorhus commented Jan 7, 2017

@hzoo Great improvement, but as you already commented, I'd like to see it as the default. Having the engine field defined means you're targeting a specific minimum Node.js version anyway, so would be less config.

.gitignore Outdated
.vscode
yarn.lock
Copy link
Member

Choose a reason for hiding this comment

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

Why do you ignore yarn.lock?

Copy link
Member Author

@yavorsky yavorsky Jan 7, 2017

Choose a reason for hiding this comment

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

@abouthiroppy yarn support will be added with the separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, to prevent commiting it every time unless it will be added, let's place it to .gitignore.

@hzoo
Copy link
Member

hzoo commented Jan 7, 2017

Having the engine field defined means you're targeting a specific minimum Node.js version anyway, so would be less config.

@sindresorhus I guess the only issue is that the node version is different than browser version if you are using it for front-end code. I can build with node 6 but still support ie?

Not sure how we could now to do it automatically since we don't know if it's a node project. However we could make a wrapper for this that assumes node?

Could make it babel-preset-env-node or ask for https://github.com/nkt/babel-preset-node @nkt? Would use the minimum of engines + current node? and have no config

@yavorsky
Copy link
Member Author

yavorsky commented Jan 7, 2017

@hzoo Maybe consider engines if no targets was specified?

# Conflicts:
#	.gitignore
#	package.json
#	src/index.js
#	src/utils.js
#	test/index.spec.js
#	yarn.lock
@yavorsky yavorsky changed the title Add node's "engines" option [WIP] Add node's "engines" option Apr 10, 2017
@nkt
Copy link

nkt commented Apr 10, 2017

Hey guys, i've noticed you asked me for babel-preset-node. I'm ready to transfer this package name to community.

@danielbayley
Copy link

danielbayley commented Apr 10, 2017

I'm currently finishing off a kind of wrapper API around this preset and the latest CoffeeScript [v2] (for easy/zero config, to hook up source maps properly and because I want it for a bunch of Atom packages…) and had started to impliment my own check for engines when it occured to me it would probably be better as a PR on here, then I found this one… Point being, I'm keen to see this merged, so anything I can do to help I'm in… what, [if any] current blockers are there?

One thing to mention also, is that in the case of Atom packages, engines will usually have an atom property specifying it's own version (which really just corresponds to a particular version of electron), so I'm wondering how to handle this… would that need to be filtered out or just be ignored by this preset anyway? It would be cool if it was automatically resolved to the correct version of electron, but that does feel kind of out of scope here…

Also +1 for getting rid of node: true

@ELLIOTTCABLE
Copy link

I'm with @yavorsky, here — I think node: true should be deprecated, in favour of an explicit (and required) string.

@k15a
Copy link

k15a commented Jun 23, 2017

Hey,

that's an awesome feature. 🎉
Could we also add support for a new option which would take the engines object directly?

{
  "presets": [
    [ "env", {
      "targets": {
        "node": { "node" : ">=0.12" },
        "browsers": [ "chrome 54" ]
      }
    }]
  ]
}

That would allow people to get the information from another source and pass it directly to the preset. I think that wouldn't be too hard because we can basically reuse the current logic.

@yavorsky
Copy link
Member Author

@k15a We can avoid node field duplicating and pass semver value directly.

{
  "presets": [
    [ "env", {
      "targets": {
        "node": ">=0.12"
      }
    }]
  ]
}

@k15a
Copy link

k15a commented Jun 23, 2017

@yavorsky Does the node field also accept ranges? I thought it currently only accepts version numbers.

@yavorsky
Copy link
Member Author

yavorsky commented Jun 23, 2017

@k15a Not yet. When we will merge this PR it would be easy to handle semver values.

@sheerun
Copy link

sheerun commented Oct 2, 2017

Maybe @yavorsky @hzoo Maybe it could first respect engines in publishConfig. It would allow for different engines for development and different for publication.

@hzoo
Copy link
Member

hzoo commented Oct 31, 2017

Thanks for the PR!

Now that babel-preset-env has stabilized, it has been moved into the main Babel mono-repo.
The move makes it much easier to release and develop in sync with the rest of Babel!

This repo will be made read-only, as all of the issues/labels have been moved over as well. Please report any bugs and open pull requests over on the main mono-repo (against master which is v7)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.