-
Notifications
You must be signed in to change notification settings - Fork 482
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
Support custom babel config #1205
Conversation
@@ -17,6 +46,9 @@ const smartGlob = require('../smart_glob.js'); | |||
* @returns results | |||
*/ | |||
function dependencyStream(indexes, config) { | |||
const babelConfig = config.babel | |||
? { configFile: path.resolve(__dirname, '../../../../', config.babel) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea here is to load the config relative to wherever documentation
was installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be easier to manage as an absolute path
@tmcw any thoughts on this? |
This looks great! The only thing I'm not sure about is the |
👍 for this! 👍 |
@tmcw, @dougalg agree, can we use an absolute path in the mean time perhaps? |
fixes issue in #996 |
Maybe |
Okay, going to merge and release a 10.x pre release so folks can test it out. |
BREAKING CHANGE: this may change babel configuration loading, and is a major change to the documentation.js approach to Babel.
Published as |
@kevinmartin the babel dependencies of documentation.js are all specified as ^7 series. There isn't an 8 series, yet, so I'm not sure how you can be using something that's newer. |
Hi! How does this resolve #1149? |
I have tried and it does not solve #1149. I also tried to add
|
I published this as a prerelease (an alpha), so |
Oh ok that make sens. Any plan for the final release? Will the |
Final release is mostly pending a few folks (it could be you!) installing the alpha and confirming that it does what you expect. |
@tmcw oh sorry, I didn't make it a priority but I didn't know it was waiting for testers. I am still surprised that we need a |
Can you at least try installing the alpha using the |
@tmcw of course, sorry to have those extra questions personal to my use case. I went through tests and I can confirm @dougalg implementation is working on the 3 projects I got it tested. I have found one potential issue if people use ReferenceError: Unknown option: .name. Check out https://babeljs.io/docs/en/babel-core/#options for more information about options. while parsing file: src/index.js This means that we cannot use
This is surprising me, we generally have one babel configuration per project, located in I am quite aware of the javascript ecosystem, what did I miss that tells you that we cannot support a default My concern is that without this automation, we will have to wrap the command and do something like this in our script: babel_config=(babel.config.js .babelrc package.json)
args=""
for cfg in "${babel_config[@]}"; do
if [[ -f $cfg ]]; then
if [[ $cfg != package.json ]]; then
args="$args --babel $cfg"
else
res=$(cat $cfg | jq .babel)
## or without jq: res=$(cat $cfg | grep '"babel": ')
## Fail, but it can be fixed by extracting the babel configuration into a temporary file
[[ -n $res ]] && args="$args --babel $cfg"
fi
fi
done
documentation build 'src/**/*.js' -f md -o test.md "$args" |
@tmcw @kopax somehow I missed all this discussion, I had totally forgotten this PR, so sorry! Thanks for merging it, I'm glad I helped. I think the ideal solution is rather than passing a babel path allow a It actually looks like maybe |
Does this support custom parser options? |
The babel option was released as part of 10.0.0, and is included in later releases, as detailed in the changelog. |
Sorry I didn't noticed it was released. People should be aware that it may break in the futur (except if we support both option) @dougalg, will you take care of the
Perhaps if I understand the concerned module and documentation I should read, I may try to help implement the feature. |
@kopax for some reason I'm not getting email notifications about discussion here. Good thing I checked back in. I don't have time at the moment, but maybe in a month or so. Regardless the existing version was merged for now, which is still a step forward. When I have time, I would like to come back to this if someone else does not take it on first. |
Has anyone managed to get Babel config working with |
Proposal:
Allow users to support their own babel configuration via new cli option:
Aims to resolve: #1149, #140
This will need some refinement of implementation, but if the overall concept is acceptable, that should be easy to resolve.