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

Make the filename option, as exposed to the plugins, consistently relative to the working directory #7956

Merged
merged 1 commit into from May 22, 2018

Conversation

@loganfsmyth
Copy link
Member

loganfsmyth commented May 17, 2018

Q                       A
Fixed Issues?
Patch: Bug Fix? Y
Major: Breaking Change? N
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Currently the opts.filename value exposed to plugins is just whatever the user passed. While it could be relative to the working directory, if Babel was passed an absolute URL, it'll be absolute.

This PR explicitly ensures the filename is a relative path based on the working directory. This also exposes an officially endorsed API for reading the working directory path.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented May 17, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7992/

@hzoo
hzoo approved these changes May 21, 2018
@loganfsmyth loganfsmyth merged commit 69cca41 into babel:master May 22, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.78% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@loganfsmyth loganfsmyth deleted the loganfsmyth:consistent-filename branch May 22, 2018
@rtsao

This comment has been minimized.

Copy link
Contributor

rtsao commented May 25, 2018

I think it's worth noting that babel-loader seems to always pass an absolute path to thefilename option (I think this.resourcePath in the webpack loader API yields an absolute path).

This change may cause specialized plugins that only run in the context of babel-loader to break if they had assumptions about this. Of course, this wasn't part of the official public API, but it was an invariant for this use case, so that assumption is not entirely unreasonable.

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented May 25, 2018

I went back and forth about whether it made sense to default to relative or absolute and I think both are entirely possible.

I ended up going with relative, but in all honesty I'd be happy to change it if people feel it'd be easier to manage. I just want it to be consistent.

@KevinGrandon

This comment has been minimized.

Copy link

KevinGrandon commented May 25, 2018

Consistency is awesome, so thanks for working on this. I think changing it to absolute would fix our use-case, as well as other folks who use babel-loader. It would be awesome if you're up for changing it to that. Happy to test out a beta/alpha version if it would help.

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented May 25, 2018

Alright, I've made a PR to make it absolute instead: #8044

@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.