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

Support in-repo addons and engines #73

Open
amk221 opened this issue Sep 27, 2017 · 17 comments
Open

Support in-repo addons and engines #73

amk221 opened this issue Sep 27, 2017 · 17 comments

Comments

@amk221
Copy link

amk221 commented Sep 27, 2017

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
Copy link
Owner

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

@bartsqueezy
Copy link

bartsqueezy commented Mar 19, 2018

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
Copy link
Owner

billybonks commented Mar 20, 2018 via email

@billybonks
Copy link
Owner

billybonks commented Mar 30, 2018

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

ember-cli/ember-cli#7713

@billybonks
Copy link
Owner

@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
Copy link

@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
Copy link
Owner

seems to be the same issue as #49, probably don't want to override cause then you loose tests, will investigate this next

@billybonks
Copy link
Owner

okay the easiest solution to this is to use this config

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

@bartsqueezy
Copy link

@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
Copy link
Owner

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
Copy link

@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
Copy link

deleemillie commented Jan 16, 2020

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
Copy link

lukemelia commented May 17, 2020

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
Copy link
Owner

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

@lukemelia
Copy link

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

@auroraborghi
Copy link

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
Copy link

rrenno commented Nov 10, 2020

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

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? 🙏

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

No branches or pull requests

7 participants