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

Pass dirname as extra metadata to preset constructor. #4834

Merged
merged 9 commits into from Feb 22, 2017

Conversation

izaakschroeder
Copy link
Contributor

@izaakschroeder izaakschroeder commented Nov 11, 2016

Q A
Bug fix? no
Breaking change? no
New feature? yes
Deprecations? no
Spec compliancy? ??
Tests added/pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

Sometimes a preset would like to know where it should resolve relative paths from (e.g. any preset that uses https://github.com/tleunen/babel-plugin-module-resolver) and this extra information makes that possible.

Sometimes a preset would like to know where it should resolve relative paths from (e.g. https://github.com/tleunen/babel-plugin-module-resolver) and this extra information makes that possible.
@codecov-io
Copy link

codecov-io commented Nov 11, 2016

Codecov Report

Merging #4834 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #4834   +/-   ##
=======================================
  Coverage   89.44%   89.44%           
=======================================
  Files         204      204           
  Lines        9949     9949           
  Branches     2689     2689           
=======================================
  Hits         8899     8899           
  Misses       1050     1050
Impacted Files Coverage Δ
.../src/transformation/file/options/option-manager.js 91.8% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07dd2b1...cc86031. Read the comment docs.

@izaakschroeder
Copy link
Contributor Author

/cc @hzoo? 😄

@hzoo hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Dec 15, 2016
@izaakschroeder
Copy link
Contributor Author

Let me know if you need anything from me @hzoo 😄

@loganfsmyth
Copy link
Member

Should we add this to plugins too?

@izaakschroeder
Copy link
Contributor Author

@loganfsmyth I wouldn't be opposed personally, some plugins might make use of it.

@motiz88
Copy link
Contributor

motiz88 commented Feb 19, 2017

I am strongly in favour of doing this for presets. It would enable babel/babel-preset-env#26 to Just Work ™️ (see babel/babel-preset-env#161 for an alternative approach without this change to core, involving passing an explicit path in preset options)

Re: Doing the same for plugins -

  1. Plugins appear to be cached rather than re-constructed for every file, so to add this we might need to undo that and risk a perf regression.
  2. Plugins can already get at this information via the transform/traversal machinery, right?

With that in mind - @hzoo @loganfsmyth, what are your thoughts about merging this? I'm thinking this just needs a test case and is otherwise ready to go.

@babel-bot
Copy link
Collaborator

Hey @motiz88! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

This adds a check for `dirname`’s existence and correctness to the
`resolve-addons-relative-to-file` test, and serves as a minimal example
of a path-aware preset.
@motiz88
Copy link
Contributor

motiz88 commented Feb 19, 2017

I've added a minimal test for this (on the back of an existing test, which is not ideal but I think reasonable here). @izaakschroeder I hope you don't mind my fiddling with your branch 😅

All done now as far as I'm concerned. @xtuc thanks for the review, want to merge this?

@izaakschroeder
Copy link
Contributor Author

Oh no by all means, happy to have this making progress @motiz88 😄

@yavorsky
Copy link
Member

Cool! Then I could make some changes to babel/babel-preset-env#161 and use path from 3-rd argument instead of explicit specifying in config.

@izaakschroeder
Copy link
Contributor Author

Let me know if you need anything else from me @loganfsmyth, @motiz88. 🎉

@izaakschroeder
Copy link
Contributor Author

Awesome! @hzoo Can you let me know what version this lands in? Thanks so much!

@izaakschroeder izaakschroeder deleted the add-directory-context-to-preset branch February 22, 2017 03:24
@hzoo
Copy link
Member

hzoo commented Feb 22, 2017

Sorry for the super long delay, next release so probably this week/next?

@izaakschroeder
Copy link
Contributor Author

No worries, that sounds great! 👌

@hzoo hzoo mentioned this pull request Feb 23, 2017
@izaakschroeder
Copy link
Contributor Author

Sorry for the nag, but any further idea when this is going out? 😄 Thanks!

izaakschroeder added a commit to metalabdesign/babel-preset-metalab that referenced this pull request Mar 13, 2017
 * Use https://github.com/babel/babel-preset-env for the majority of compatbility plugins.
 * Detect proper working directory for path resolution with babel/babel#4834.
 * Add tests for path resolution.
 * Add plugins for `react` optimizations.
 * Update dependencies to latest versions.
existentialism pushed a commit that referenced this pull request May 19, 2017
* Pass `dirname` as extra metadata to preset constructor.

Sometimes a preset would like to know where it should resolve relative paths from (e.g. https://github.com/tleunen/babel-plugin-module-resolver) and this extra information makes that possible.

* Test for `dirname` passed into preset constructor

This adds a check for `dirname`’s existence and correctness to the
`resolve-addons-relative-to-file` test, and serves as a minimal example
of a path-aware preset.
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants