Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

web/node: Easier targeting via options.targets #438

Merged
merged 5 commits into from
Nov 20, 2017

Conversation

timkelty
Copy link
Contributor

It is currently rather verbose to override the targets.browsers option for babel-preset-env, and it requires you to overwrite the whole options object rather than just specifying browsers: https://neutrino.js.org/presets/neutrino-preset-web/#preset-options

This PR adds options.browsers to be able to specify directly.

Furthermore, when babel-preset-env@7.0 is released (currently at beta.32), this will make it easier for users that want to use a browserslist config file, as the new version of babel-preset-env will look for those automatically. In that case, we can support passing false or undefined to have babel-preset-env/browserslist do its thing.

@eliperelman
Copy link
Member

The reason I have shied away from doing something like this has to do with parity of the Node.js preset. The phrase targets applies to both. Maybe I'm over-thinking this too, and it's OK to have a browsers option for web and a node option for node.

Using targets gives a common configuration, with the tradeoff being more verbose nesting/more complex nesting:

['@neutrinojs/web', {
  targets: {
    browsers: [/* ... */]
  }
}]

['@neutrinojs/node', {
  targets: {
    node: /* ... */
  }
}]

Using browsers/node gives a simpler and more targeted configuration per preset, with the tradeoff being a divergent configuration, which isn't really a problem:

['@neutrinojs/web', {
  browsers: [/* ... */]
}]

['@neutrinojs/node', {
  node: /* ... */
}]

@timkelty
Copy link
Contributor Author

Yeah I'd be fine with just exposing options.targets instead. In which case, we should add the option to both node and web preset. Do you agree?

@eliperelman
Copy link
Member

Let's get some more opinions. @mozilla-neutrino/core-contributors any thoughts?

@timkelty
Copy link
Contributor Author

I like the idea of maintaining parity with @neutrinojs/node.

I updated my PR to pass targets instead - to both node and web.

@timkelty timkelty changed the title web: Easier browser targeting via options.browsers web/node: Easier targeting via options.targets Nov 16, 2017
Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

This looks good to me. @mozilla-neutrino/core-contributors anyone else have any concerns?

@eliperelman
Copy link
Member

@timkelty could you also add this option to the docs?

@timkelty
Copy link
Contributor Author

Will do.

@timkelty
Copy link
Contributor Author

Docs updated.

targets: {
node: '6.10'
}

// Add additional Babel plugins, presets, or env options
babel: {
// Override options for babel-preset-env, showing defaults:
presetEnv: {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, there is no more presetEnv here, must be a typo. Can you update this to:

babel: {
  presets: [
    ['babel-preset-env', {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yep - I wondered about that.

Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks good! As soon as we get that existing typo fixed, we can merge this.

@edmorley
Copy link
Member

Good idea! I was only thinking the other day that our configs were pretty verbose just to specify a reduced set of targets.

So long as the docs/ and packages/ copy of the docs are in sync this looks great :-)

@timkelty
Copy link
Contributor Author

Docs are fixed!

@eliperelman eliperelman merged commit caf065c into neutrinojs:master Nov 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants