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

fix(resolve-rc): look for babel in package.json #456

Closed

Conversation

mastilver
Copy link

@mastilver mastilver commented May 30, 2017

Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

If this PR contains a breaking change, please describe the following...

  • Impact:
  • Migration path for existing applications:
  • Github Issue(s) this is regarding:

Other information:

@danez
Copy link
Member

danez commented Jun 1, 2017

Awesome thank you very much.
By looking at the code I noticed that babelrc.js is not supported yet also (which is coming with babel 7).
Instead of further modifying this code, what do you think about using https://github.com/tleunen/find-babel-config? It already supports the new file. And would make the babel-loader code more easy by outsourcing this part. We would still need to do the caching.

@alexander-akait
Copy link
Contributor

alexander-akait commented Jun 8, 2017

@danez

We would still need to do the caching.

Now it is not work as expected. Step:

  1. Run webpack in watch mode.
  2. Just edit config file - rebuilding process not started.

How to solve:

  1. Use this.addDependency for configuration file (.babelrc and etc), how it is do postcss-loader https://github.com/postcss/postcss-loader/blob/master/lib/index.js#L93

@mastilver
Copy link
Author

@evilebottnawi I think what @danez was saying is for performance sake find-babel-config should have caching

What you mention is a different issue and from my point of view is not an issue
webpack only watch "code" files not config files, if you update your .babelrc you are required to restart webpack

@alexander-akait
Copy link
Contributor

alexander-akait commented Jun 8, 2017

@mastilver @danez This is fundamentally wrong and breaks the logic of watching and caching.
Each loader using config files should be add their to watch graph.

From webpack channel as say @sokra:

sokra [4:54 PM] 
configuration files -> loaders should add these files to their dependencies (with `this.addDependency`). dependencies' mtimes are stored by the cache-loader

Also it is break loader before, example cache-loader, actually loaders using before babel-loader, which a lot more and many of their also can improve own cache. All this makes the cache invalid and it is wrong look at watch graph.

@mastilver
Copy link
Author

@evilebottnawi I take back what I said but can you open an issue about it, this PR is not related to your issue

@danez
Copy link
Member

danez commented Jun 16, 2017

Thanks this was merged with d8b73c0

@danez danez closed this Jun 16, 2017
@mastilver mastilver deleted the read-package-json-babel-config branch June 16, 2017 16:06
@mastilver
Copy link
Author

@danez Thank you for taking care of it :) , don't have much time atm

@ismay
Copy link

ismay commented Jun 17, 2017

@danez This hasn't yet been released on npm right?

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

Successfully merging this pull request may close these issues.

4 participants