billybonks / ember-cli-stylelint

Adds styleint to your ember app, to lint all kinds of css
MIT License
48 stars 13 forks source link

Support in-repo addons and engines #73

Open amk221 opened 6 years ago

amk221 commented 6 years ago

With a config like:

stylelint: {
  syntax: 'scss',
  testFailingFiles: true,
  includePaths: [
    'lib/engine1/addon/styles',
    'lib/engine2/addon/styles',
    'lib/inrepoaddon1/app/styles',
    'lib/inrepoaddon2/app/styles'
  ]
}

I get this error

Error: Merge error: file addon.stylelint-test.js exists in tmp/broccoli_merge_trees-input_base_path-vQ7dMwvy.tmp/1 and tmp/broccoli_merge_trees-input_base_path-vQ7dMwvy.tmp/2
Pass option { overwrite: true } to mergeTrees in order to have the latter file win.
    at BroccoliMergeTrees._mergeRelativePath (node_modules/broccoli-merge-trees/index.js:278:17)
    at BroccoliMergeTrees.build (node_modules/broccoli-merge-trees/index.js:83:24)
    at node_modules/broccoli-plugin/read_compat.js:93:34
    at tryCatch (node_modules/rsvp/dist/rsvp.js:525:12)
    at invokeCallback (node_modules/rsvp/dist/rsvp.js:538:13)
    at publish (node_modules/rsvp/dist/rsvp.js:508:7)
    at flush (node_modules/rsvp/dist/rsvp.js:2415:5)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)
billybonks commented 6 years ago

ill take a look over the weekend to see how i can get it working for engines let you know

bartsqueezy commented 6 years ago

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 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.

billybonks commented 6 years ago

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 notifications@github.com 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 https://github.com/billybonks/ember-cli-stylelint/issues/73#issuecomment-374380789, or mute the thread https://github.com/notifications/unsubscribe-auth/ABkGyOJI5So5dZkjAAyExNbenhxCDFgDks5tgCAugaJpZM4Pl92r .

billybonks commented 6 years ago

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 includePaths since that config is a workaround

https://github.com/ember-cli/ember-cli/pull/7713

billybonks commented 6 years ago

@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 :)

bartsqueezy commented 6 years ago

@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...

Merge error: file addon.stylelint-test.js exists 
Pass option { overwrite: true } to mergeTrees in order to have the latter file win.

I think the final remaining change is to add { overwrite: true } as the 2nd argument when calling mergeTrees. After making that change locally, builds were back up and running again.

billybonks commented 6 years ago

seems to be the same issue as https://github.com/billybonks/ember-cli-stylelint/issues/49, probably don't want to override cause then you loose tests, will investigate this next

billybonks commented 6 years ago

okay the easiest solution to this is to use this config

    stylelint: {
      includePaths: [
        'lib',
        ]
    }
bartsqueezy commented 6 years ago

@billybonks, I have multiple engines and addons under /lib. Does it make a difference if you reference each directory directly instead of just the parent?

stylelint: {
  generateTests: true,
  includePaths: [
    'lib/engine01/addon/styles',
    'lib/engine02/addon/styles',
    'lib/engine03/addon/styles',
    'lib/addon/app/styles'
  ]
}

With this config, I still get the merge error during a build.

billybonks commented 6 years ago

right now if you use lib/engine01/addon/styles and you have file style.scss the output will be style.test.js, now if lib/engine02/addon/styles also has style.scss it will output the same file in the same virtual directory. so if you use the overwrite config you actually loose one of the tests.

but if you reference the parent lib then the above 2 files in the virtual directory will be lib/engine01/addon/styles/style.test.js lib/engine02/addon/styles/style.test.js

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 lib should be totally safe, the only thing i would look out for is if it increases your built times, because it broccoli will do a bit more processing but i think it shouldn't be very bad.

bartsqueezy commented 6 years ago

@billybonks, I confirmed this working on my end. Specifying only the parent lib dir in the includePaths option showed a negligible difference in load times. Thank you again for getting a workaround merged! Hoping we get some traction in ember-cli repo for addressing this correctly 🤞

deleemillie commented 4 years ago

This continues to be a problem. We are using the following addon combinations:

    "ember-cli-stylelint": "3.0.2",
    "ember-cli-template-lint": "2.0.0",
    "stylelint": "13.0.0",
    "stylelint-config-standard": "19.0.0",
    "stylelint-order": "4.0.0"

We have the following settings;

stylelint: {
  includePaths: ['addon']
},

We receive the below error:

Build Error (BroccoliMergeTrees)

[BroccoliMergeTrees] error while merging the following:
  1.  [SimpleConcatConcat]
  2.  [SimpleConcatConcat]
Merge error: file true.stylelint-test.js exists in C:\Users\[user]\AppData\Local\Temp\broccoli-37716Flv0TMg2N5eI\out-040-simple_concat_concat and C:\Users\[user]\AppData\Local\Temp\broccoli-37716Flv0TMg2N5eI\out-045-simple_concat_concat
Pass option { overwrite: true } to mergeTrees in order to have the latter file win.

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:

/addon/component/mycomponent1/_style.scss
/addon/component/mycomponent2/_style.scss
/addon/component/mycomponent3/childcomponent/_style.scss
/addon/css/bundle.scss

The workarounds listed above do not work for us.

Using version 2.2.0 of this addon allows everything to work.

lukemelia commented 4 years ago

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 app/styles as well as tests/dummy/styles. The error was:

➜  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
billybonks commented 4 years ago

Hey, will take a look at this again on the weekend.

lukemelia commented 4 years ago

@billybonks thanks. happy to remote pair on it if you want. you can ping me on discord.

auroraborghi commented 4 years ago

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 ember-cli-stylelint in an addon, I received the following error:

Build Error (BroccoliMergeTrees)

[BroccoliMergeTrees] error while merging the following:
  1.  [SimpleConcatConcat]
  2.  [SimpleConcatConcat]
  3.  [SimpleConcatConcat]
Merge error: file true.stylelint-test.js exists in /var/folders/cq/gz2xyllj2714wz0wtd96x7700000gp/T/broccoli-65859uoOyr5SOMprb/out-023-simple_concat_concat and /var/folders/cq/gz2xyllj2714wz0wtd96x7700000gp/T/broccoli-65859uoOyr5SOMprb/out-028-simple_concat_concat
Pass option { overwrite: true } to mergeTrees in order to have the latter file win.

As a workaround, I went to ./node_modules/ember-cli-stylelint/index.js and modified the line:

return mergeTrees(linted);

to:

return mergeTrees(linted, {overwrite: true});

which ultimately fixed the error. Hope this helps others who might run into this issue!

rrenno commented 3 years ago

I've run into the same issue with an internal addon. I think the correct solution is to set group: false.

However this merge https://github.com/billybonks/ember-cli-stylelint/commit/33b63a832bce64487c759a584446319f0e4c3c41 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? 🙏