-
Notifications
You must be signed in to change notification settings - Fork 13
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
Support in-repo addons and engines #73
Comments
ill take a look over the weekend to see how i can get it working for engines let you know |
I've run into the same issue. I have two in-repo engines and one in-repo addon that contains shared classes...
Without providing any configuration options, I noticed that the conditional check in lintTree(type, true) {
if (type === 'app' && tree.destDir === '/') {
// unchanged code
return mergeTrees(linted, { overwrite: true });
}
} And adding the following to the parent app's stylelint: {
generateTests: true,
includePaths: [
'lib/engine-one/addon/styles',
'lib/engine-two/addon/styles',
'lib/shared-classes/app/styles'
]
} ...the build is now successful and all I don't know if this is the proper way to address this but it has fixed our build issues. Figured I'd post this solution as an potential option and open it up to feedback. I would be happy to submit a merge request as well. |
ye would like to have a pull request for this, and/pr you able to write a
few unit tests as well/ make a demo app with this configuration working.
…On Tue, Mar 20, 2018 at 5:16 AM Steve Bartnesky ***@***.***> wrote:
I've run into the same issue. I have two in-repo engines and one in-repo
addon that contains shared classes...
/lib
/engine-one
/engine-two
/shared-classes
Without providing any configuration options, I noticed that the
conditional check in lintTree passes twice, once for the main app and
another for the in-repo-addon. This is why it's presenting the error that
everyone is seeing. After making the following changes to index.js...
lintTree(type, true) {
if (type === 'app' && tree.destDir === '/') {
// unchanged code
return mergeTrees(linted, { overwrite: true });
}
}
And adding the following to the parent app's ember-cli-build.js file...
stylelint: {
generateTests: true,
includePaths: [
'lib/engine-one/addon/styles',
'lib/engine-two/addon/styles',
'lib/shared-classes/app/styles'
]
}
...the build is now successful and all .scss files are linted correctly.
I don't know if this is the proper way to address this issue but it has
fixed our build issues. Figured I'd post this solution as an potential
option and open it up to feedback. I would be happy to submit a merge
request as well.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#73 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABkGyOJI5So5dZkjAAyExNbenhxCDFgDks5tgCAugaJpZM4Pl92r>
.
|
Just to update everyone on this thread, making some pull requests to ember-cli that will make styles first class citizens for linting that should resolve all quirks but at the same time it means i will deprecate |
@bartsqueezy https://github.com/billybonks/ember-cli-stylelint/releases/tag/2.1.0 if you upgrade to this version your includedPaths should work now without changing the source please let me know :) |
@billybonks, thank you for spending the time to get this issue corrected. Much very appreciated! I pulled v2.1.0 and I still get presented with an error message...
I think the final remaining change is to add |
seems to be the same issue as #49, probably don't want to override cause then you loose tests, will investigate this next |
okay the easiest solution to this is to use this config
|
@billybonks, I have multiple engines and addons under
With this config, I still get the merge error during a build. |
right now if you use but if you reference the parent so they wont override. in theory i can move them after i generate the tests, but won't be able to work on that until 18april since i will be offline. using the parent |
@billybonks, I confirmed this working on my end. Specifying only the parent |
This continues to be a problem. We are using the following addon combinations:
We have the following settings;
We receive the below error:
We have multiple _style.scss files against multiple folders and components, as well as files that group them together in a global directory. Basically, we have styling per component. Our directory structure looks something like:
The workarounds listed above do not work for us. Using version |
Howdy @billybonks! I ran into this today trying to upgrade from 2.2.0 to 4.0.0. I got the error in an internal addon (not an in-repo addon, just a regular addon that happens to be private to our company) that has scss files in both ➜ yapp-ember-kit git:(chore/release-it) ✗ ember s
Build Error (BroccoliMergeTrees)
[BroccoliMergeTrees] error while merging the following:
1. [SimpleConcatConcat]
2. [SimpleConcatConcat]
Merge error: file true.stylelint-test.js exists in /var/folders/vs/8wls8rjn4gqd4m2ydxywp6v00000gn/T/broccoli-92843PN6bPkEhpOtY/out-039-simple_concat_concat and /var/folders/vs/8wls8rjn4gqd4m2ydxywp6v00000gn/T/broccoli-92843PN6bPkEhpOtY/out-043-simple_concat_concat
Pass option { overwrite: true } to mergeTrees in order to have the latter file win.
Stack Trace and Error Report: /var/folders/vs/8wls8rjn4gqd4m2ydxywp6v00000gn/T/error.dump.2d289c976fbaf6253f66e56347290ca4.log |
Hey, will take a look at this again on the weekend. |
@billybonks thanks. happy to remote pair on it if you want. you can ping me on discord. |
Hallo @billybonks! I just wanted to add to this thread and state that this error is still prevalent in version 4.0.0, and also to add a quick workaround for anyone who is running into this issue. While working on adding
As a workaround, I went to
to:
which ultimately fixed the error. Hope this helps others who might run into this issue! |
I've run into the same issue with an internal addon. I think the correct solution is to set However this merge 33b63a8 seems to prevents anything but the default. I assume the intention was to default only if undefined this.styleLintOptions.group = this.styleLintOptions.group === undefined ? true : this.styleLintOptions.group; vs always forcing true this.styleLintOptions.group = true; @billybonks can you confirm? Any chance this could be patched? 🙏 |
With a config like:
I get this error
The text was updated successfully, but these errors were encountered: