-
-
Notifications
You must be signed in to change notification settings - Fork 54
blueprint remove old files #92
blueprint remove old files #92
Conversation
…ripping Ember CLI: Production code stripping
module.exports = { | ||
// ... | ||
|
||
oldFilesToRemove: [ |
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.
It is possible to want to programmatically choose which files are to be deleted? For example, one may want to remove files only if present, or if a specific add-on is present or configuration is set. Given that, should this be declarative or imperative? (I'm not trying to say it one or the other, if possible declarative would be my preference).
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.
It makes sense to me to dual function this property as taking an array or a function that returns an array. Is that what you're talking about?
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.
its mostly because it is currently a property on the class, and not the instance. Which is a tad funky (although nice for simple addon mains)
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.
I don't follow. Where do you suggest it be moved to?
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.
As this this implementation will not land before Jan 1, 2017. I believe we can use ES6 feature:
module.exports = {
get existingFilesToRemove() {
return [ /* ... */ ]
}
}
This should enable both a reasonable API, and ensure the property exists on the instance and not the class.
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.
yup confirmed node v4.x supports what we would need (if this approach is taken)
node-set v4
✓ node.current = v4.6.2
s/s/broccoli-viz ╍{} node
> a = { get foo() { return []; }}
> a.foo # => []
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.
I will update.
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.
@stefanpenner - That is also available in 0.12 (its just ES5 getters no?).
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.
@rwjblue yup!
|
||
We'd like a declarative API for blueprints to delete files instead of only | ||
create. It would be essentially syntactic sugar for removing the file yourself | ||
in an `afterInstall` hook. It would be an array on the blueprint's `index.js`. |
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.
How do the deletion + addition rules compose?
For example:
- default blueprint: adds
.jshintrc
- eslint blueprint: removes
.jshintrc
Scenarios:
user runs the eslint blueprint for the first time, output would be:
file remove: .jshintrc
user then later runs ember init
One approach would result in:
file add: .jshintrc
--- eslintrc addon
file remove: .jshintrc
One approach would result in no output
One may output exactly what happened:
file ensured remove: .jshintrc
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.
I was thinking it would be as basic as possible, and let git handle anything more complicated.
- eslint removes .jshintrc
- git commit
- some time passes
- ember init adds it back
- git tells you this file you don't want was added back, so you manually remove the file
If we started composing, I think there would be many corner cases with a lot of complexity. My goal is to get old files out of the project that until now were invisible to you. With my steps above, it is now visible to you that it was removed for some reason, even though there are some extra steps to get rid of it again.
@stefanpenner I've addressed concerns. This is ready for more scrutiny, or approval. Possible bikeshedding on naming though. cc @ember-cli/core |
|
||
# Drawbacks | ||
|
||
The only reason to not do this is to hold out for a large blueprint reworking. |
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.
I wouldn't worry about the blprnt extraction. That will take some more effort due to how the test harnesses work. And i don't mind backporting any work your doing there rather then blocking the world because i haven't finished that yet.
|
||
We'd like an API for blueprints to delete files instead of only | ||
create. It would be essentially syntactic sugar for removing the file yourself | ||
in an `afterInstall` hook. It would be a returned array on the blueprint's `index.js`. |
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.
So today, file mutations are intended to be transactional. Specifically, it is possible cancel / say no abort a blueprint and abort. As described, this appears like it may not participate in that, is that the case?
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.
I lied then 😛. It is not sugar for afterInstall
after all, only in concept, because I believe I am still participating in the transactional system here: https://github.com/ember-cli/ember-cli/pull/6472/files#diff-3b6ee55eb49134b6b858dea4972b3117R736
# Alternatives | ||
|
||
The key name can be bikeshed. I chose `oldFilesToRemove` to be verbose and | ||
explicit, but it can be changed. |
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.
@kellyselden oldFilesToRemove
indicates that this should only be used for files there were previously part of the blueprint. what about other scenarios where files should be removed? e.g. eslint removing jshint files? I'm wondering if that key should be more generic
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.
Great point. One main goal of this RFC is to settle on the best name.
This was discussed in the Ember CLI team meeting, and there are no more pressing concerns. This is now in Final Comment Period for the next 7 days. |
|
||
# Motivation | ||
|
||
We want to eliminate the noise of having old files laying after updating |
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.
nit: grammar: "having old files lying around
after"
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.
grammar
How would you register one of those files that include tokens in them Maybe the oldFilesToRemove should be allowed to have tokens (same tokens as the ones available in the blueprint's files directory). Edit: I was thinking about the correct thing. The question still remains. Example: A file like |
Agreed, but this RFC doesn't prevent that (and it should be pretty easy to add in a followup PR). |
This has been in final comment period for quite a bit, and no new complications have been raised. Thank you all for reviewing and contributing! |
This reverts commit be142aa, reversing changes made to b90f860. This is being reverted due to user experience issues when the `bower.json` file is no longer part of the blueprint. As things exist prior to a revert, any user upgrading from a version of ember-cli prior to 2.11 would never be instructed to update their `bower.json` to remove entries for `ember`, `ember-cli-shims`, etc. To bring this back, we need to land the ability for a blueprint to indicate that a file should be deleted as part of the `ember init` process. This was RFC'ed (ember-cli/rfcs#92) and should hopefully land in 2.13 (current ember-cli#master branch).
Revert changes removing `bower.json` from default blueprints. This reverts commit be142aa, reversing changes made to b90f860. This is being reverted due to user experience issues when the `bower.json` file is no longer part of the blueprint. As things exist prior to a revert, any user upgrading from a version of ember-cli prior to 2.11 would never be instructed to update their `bower.json` to remove entries for `ember`, `ember-cli-shims`, etc. To bring the changes being reverted back, we need to land the ability for a blueprint to indicate that a file should be deleted as part of the `ember init` process. This was RFC'ed in ember-cli/rfcs#92 and the implementation is pending in #6472 and should hopefully land in 2.13 (current ember-cli#master branch). This was discussed at the ember-cli meeting on 2017-01-26.
…-files blueprint remove old files
Rendered
Reference PR ember-cli/ember-cli#6472