-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
content rules from the JS config that are also covered by the automatic source detection should not be migrated to CSS
#14714
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
content rules from the JS config that are also covered by the automatic source detection should not be migrated to CSS
#14714
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @philipp-spiess and the rest of your teammates on |
|
Yo what is this branch name, let me see if I can truncate this in Graphite 🙈 |
49a0983 to
5c25f28
Compare
| warn( | ||
| 'The `content` configuration `${content}` is already included in the automatic content file discovery and will not be migrated.', | ||
| ) |
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.
Actually seeing this I think I'd rather just not log anything, it almost reads like the upgrade couldn't be completed and I'm not sure how to say it more clearly without it being too verbose.
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.
@adamwathan Yeah I think this makes sense.
| function listAutoContentFiles(base: string) { | ||
| let scanner = new Scanner({ detectSources: { base } }) | ||
| scanner.scan() | ||
| return scanner.files | ||
| } | ||
|
|
||
| function listSourceContentFiles(source: { base: string; pattern: string }): string[] { | ||
| let scanner = new Scanner({ sources: [source] }) | ||
| scanner.scan() | ||
| return scanner.files | ||
| } |
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.
Should we avoid the word "content" for code that's just looking at the modern stuff?
Are these names clear enough?
| function listAutoContentFiles(base: string) { | |
| let scanner = new Scanner({ detectSources: { base } }) | |
| scanner.scan() | |
| return scanner.files | |
| } | |
| function listSourceContentFiles(source: { base: string; pattern: string }): string[] { | |
| let scanner = new Scanner({ sources: [source] }) | |
| scanner.scan() | |
| return scanner.files | |
| } | |
| function autodetectedSourceFiles(base: string) { | |
| let scanner = new Scanner({ detectSources: { base } }) | |
| scanner.scan() | |
| return scanner.files | |
| } | |
| function patternSourceFiles(source: { base: string; pattern: string }): string[] { | |
| let scanner = new Scanner({ sources: [source] }) | |
| scanner.scan() | |
| return scanner.files | |
| } |
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.
Sounds good. I went with content in the first iteration since we are looking at the content array and this is the content field migration, but yeah in terms of v4, the term content does not really exists.
c54fc93 to
eb7a747
Compare
…atic source detection should not be migrated to CSS
Co-authored-by: Adam Wathan <[email protected]>
eb7a747 to
c0de36d
Compare

This PR changes the migration of
contentrules in the JS config to CSS codemods.When a
contentrule is processed which matches files that are also matched by the automatic content discovery in v4, we do not need to emit CSS for that rule.Take, for example this v3 configuration file:
Provided the base directories match up, the first rule will also be covered by the automatic content discovery in v4 and thus we only need to convert the second rule to CSS: