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

hash current Babel options state to cache path #62

Closed
deepsweet opened this issue May 15, 2015 · 19 comments
Closed

hash current Babel options state to cache path #62

deepsweet opened this issue May 15, 2015 · 19 comments

Comments

@deepsweet
Copy link

hi.

for example, I have .babelrc:

{
  "optional": [ "runtime", "asyncToGenerator" ],
  "blacklist": [ "regenerator" ],
  "stage": 0,
  "env": {
    "development": {
      "plugins": [ "typecheck" ]
    }
  }
}

and I keep getting the same cached version if I just want to rebuild my bundle from development env to production w/o any files changes.

rm babel-loader-* from require('os').tmpdir() helps once :)

so imo current options state needs to be stored somewhere here, but I can't figure out how to get it from babel to make a PR.

@Couto
Copy link
Member

Couto commented May 15, 2015

There's a slightly different version on the develop branch. Do you want to give it a go and tell me if it still happens? (I mean using the cacheIdentifier)

@Couto
Copy link
Member

Couto commented May 23, 2015

I'm closing this due inactivity.
It's possible to use the cacheIdentifier as a cache buster, by saving any string in there.

Anything else just give it a shout and I'll re-open the issue.

@Couto Couto closed this as completed May 23, 2015
@taion
Copy link
Contributor

taion commented Jun 15, 2015

This came up for us on react-bootstrap/react-bootstrap#840 - it'd be great if cacheIdentifier could use more than just the versions, and also look at the Babel configuration as well, in the case of e.g. changes to .babelrc.

@Couto
Copy link
Member

Couto commented Jun 15, 2015

If you want to change the cacheIdentifier everytime the .babelrc changes... why don't you just JSON.stringify() the contents of .babelrc into the loader options? It should accept any string you want.

Either that or I might not be understanding the problem correctly.

@taion
Copy link
Contributor

taion commented Jun 15, 2015

I could certainly do that, but I think it would be nice if babel-loader took care of this automatically.

The reason I think that's the case is because we saw an issue where we added "runtime" to "optionals" in .babelrc, but we wouldn't see the webpack output change, because the cache wasn't invalidated.

I believe this kind of condition where a user changes .babelrc and then expects to see the webpack output change despite using cacheDirectory is common enough that it should be generally supported.

@Couto
Copy link
Member

Couto commented Jun 15, 2015

I can easily agree that the user would expect the cache to be invalidated, however I'm not sure if it's the loader's responsibility to actually gather info from .babelrc, but I can at least try....

Anyway, I'll try to work on this tomorrow morning, since it's quite late in here.
Meanwhile I'm open to PRs

@Couto Couto reopened this Jun 15, 2015
@taion
Copy link
Contributor

taion commented Jun 15, 2015

Sadly it's going to be a bit messy, because I think you'd have to use https://github.com/babel/babel/blob/master/src/babel/transformation/file/options/resolve-rc.js , which is likely to make @sebmck unhappy.

But I don't know if there's a better option.

@sebmck
Copy link
Contributor

sebmck commented Jun 15, 2015

If you hotlink to my internal APIs then I will cry

@Couto
Copy link
Member

Couto commented Jun 16, 2015

@sebmck you and me... specially because babel moves really fast which would mean constant updates on my part. Might try to duplicate the resolve rc functionality into the loader.

but anyway... so far it's just me talking, I will try to mess around to see what I can do.

@Couto
Copy link
Member

Couto commented Jun 18, 2015

Ok... sorry for taking so long, I'm having a hard time to find some free time.

A proof of concept can be found on the feature/resolve-rc... it's hardly tested, but it seems to work on my examples. I was wondering if you're up to test it on your project, and I'm open for opinions.

The option to use .babelrc as cacheIdentifier is opt-in, so you'll have to specify that you want to include it like so:

loaders: [{
  test: /\.js$/,
  loader: 'babel',
  exclude: /node_modules/,
  query: {
    babelrc: true,
    cacheDirectory: true,
  }
}]

@taion
Copy link
Contributor

taion commented Jun 18, 2015

This is really cool, thanks. What's the benefit of making this opt-in rather than opt-out? It seems like one would usually strongly prefer to have this feature, no?

BTW, might make sense to use https://www.npmjs.com/package/json-stable-stringify instead of the standard JSON.stringify to ensure that hashes don't change spuriously.

@Couto
Copy link
Member

Couto commented Jun 18, 2015

The reason why it's opt-in is mostly to avoid a new unexpected behaviour for the ones already using the cache feature.

Still... this is a work in progress. I'm happy for any input :)

On June 18, 2015 5:45:34 PM GMT+01:00, Jimmy Jia notifications@github.com wrote:

This is really cool, thanks. What's the benefit of making this opt-in
rather than opt-out? It seems like one would usually strongly prefer to
have this feature, no?

BTW, might make sense to use
https://www.npmjs.com/package/json-stable-stringify instead of the
standard JSON.stringify to ensure that hashes don't change
spuriously.


Reply to this email directly or view it on GitHub:
#62 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@taion
Copy link
Contributor

taion commented Jun 20, 2015

I really like this, BTW. I think I'd pretty much use it everywhere.

@taion
Copy link
Contributor

taion commented Jun 24, 2015

I like this feature a lot and would switch to using it in all cases where I'm currently using cache directory, as it will allow me to get rid of fragile logic for bypassing the cache only for particular builds. Are there any concerns preventing it from being merged?

@Couto
Copy link
Member

Couto commented Jun 25, 2015

I've just merged this feature.
The .babelrc is included automatically, no opt-in required.
You can find this on npm with the version 5.2.0

@taion
Copy link
Contributor

taion commented Jun 25, 2015

Awesome. Thanks!

@Couto
Copy link
Member

Couto commented Jun 26, 2015

I'm closing this, since it has been merged.
If you find any errors/bugs please reopen :)

@Couto Couto closed this as completed Jun 26, 2015
@SimenB
Copy link

SimenB commented Jul 28, 2015

I haven't tested it, but does this work if the .babelrc-config is in package.json? https://babeljs.io/docs/usage/babelrc/#use-via-package-json

@Couto
Copy link
Member

Couto commented Jul 28, 2015

At the moment it doesn't, however a PR should not be too hard :)

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

No branches or pull requests

5 participants