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

Incorporate cache keys. #15

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Incorporate cache keys. #15

merged 2 commits into from
Aug 10, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 10, 2016

Provide baseDir and pass through second argument as cacheKey.

This is backwards compat to 0.0.5...

@Turbo87
Copy link
Member

Turbo87 commented Aug 10, 2016

@rwjblue could you crosslink why you're doing this?

@rwjblue
Copy link
Member Author

rwjblue commented Aug 10, 2016

The upstream usage of baseDir and cacheKey functions is being added here: babel/broccoli-babel-transpiler#89.

Right now, the following scenario will leave you with a cached JS file that is not properly invalidated:

ember new foo
cd foo
ember g component foo
ember test
bower install --save ember#alpha
ember test

The last ember test will fail because the template compilation output is different between 1.10 - 2.8 and the alpha, and since this plugin rewrites the transpiled output to include the result of Ember.Handlebars.compile but the template compiler itself isn't part of the broccoli-babel-transpiler's cache keys we end up with embedded templates that will only be recomputed if you change the file (to invalidate the cache another way).

babel/broccoli-babel-transpiler#89 is aiming to solve this, and will warn/deprecate/etc any instances where the baseDir is not present.

@Turbo87
Copy link
Member

Turbo87 commented Aug 10, 2016

okay LGTM, could you maybe add a few short code comments and/or a sentence about it to the README?

@stefanpenner @pangratz r+?

@rwjblue
Copy link
Member Author

rwjblue commented Aug 10, 2016

@Turbo87 - Update README and added a couple inline comments explaining why this is being done.

@Turbo87
Copy link
Member

Turbo87 commented Aug 10, 2016

I have to admit I'm not entirely happy with the approach because it is very uncommon to pass functions to Babel plugins and even more so to Babel 6 plugins where they are usually referenced in a .babelrc JSON file that has no support for functions. I understand though that we are already passing in the main precompile function and that is why I have mixed feelings about it...

does cacheKey have to be a function?

should we actually bundle/depend on the handlebars precompiler in this plugin instead of passing it in like we currently do?

@stefanpenner
Copy link
Contributor

@Turbo87 would love your 👀 on this, and ideas to improve it.

@rwjblue
Copy link
Member Author

rwjblue commented Aug 10, 2016

even more so to Babel 6 plugins where they are usually referenced in a .babelrc JSON file that has no support for functions

In the upstream changes this case is handled properly in the default scenario you describe. When the config includes an array (where options are the second item in the array), those options are used for the cache key by default. If a function is found in that options hash, it is expected to include its own cacheKey/baseDir.

does cacheKey have to be a function?

options.cacheKey does not need to be a function, I only made it a function to be the same as what the plugin itself is expected to provide.

should we actually bundle/depend on the handlebars precompiler in this plugin instead of passing it in like we currently do?

We cannot bundle here, as it is dependent on the Ember version in use by the app itself (and changes per build).

I understand though that we are already passing in the main precompile function and that is why I have mixed feelings about it.

I'm not sure what else we can do, but we must do something. I'm definitely open to ideas...

Robert Jackson added 2 commits August 10, 2016 12:10
This works in conjunction with the changes made upstream in
broccoli-babel-transpiler to allow individual plugins to augment the
cache key.
@rwjblue
Copy link
Member Author

rwjblue commented Aug 10, 2016

does cacheKey have to be a function?

Updated to make options.cacheKey a primitive value.

@rwjblue
Copy link
Member Author

rwjblue commented Aug 10, 2016

Chatted with @Turbo87 and he is not super opposed to shipping this for now and coming back to fix things when Ember itself can ship as an NPM package (and we can avoid passing this function around like this).

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

Successfully merging this pull request may close these issues.

None yet

3 participants