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

Linting in addon #14

Closed
wants to merge 3 commits into from
Closed

Linting in addon #14

wants to merge 3 commits into from

Conversation

synaptiko
Copy link

@synaptiko synaptiko commented Feb 2, 2018

Hey guys,

we started using mono-repository with nested addons for development of our app and this started to be a problem. I found there is already issue for it and even branch with a fix. So I revamped it, rebased and fixed one issue I found.

The issue was in usage of fs.stat instead of fs.statSync in forEach loop. It worked regardless but I think the intention was different so I fixed it. I replaced plain paths with new Funnel as I noticed this is change done in master too.

I also tried to write test for addon logic but I failed miserably. I'm not much familiar with Broccoli, Funnels and such stuff. If there is anybody who would help me with a test I would appreciate it.

So do you mind to review, merge & release a new version? :-) Thank you very much!

Fixes #3

@RobbieTheWagner
Copy link
Collaborator

Thanks @synaptiko! Does this work if you link it to your app and run? If all appears to be working on your end, I am fine with merging. We don't do much maintenance on this. I switched to using ember-cli-stylelint myself.

@synaptiko
Copy link
Author

@rwwagner90 thanks for a good hint to test this scenario. There is actually a problem in case you have isDevelopingAddon() { return true; } in a nested addon and you run ember build from the app. The scss files are checked multiple times. I'll debug it and hopefully find some fix to this. But without the isDevelopingAddon flag it works fine.

@RobbieTheWagner
Copy link
Collaborator

@synaptiko yeah I believe that was the reason we had not merged these changes in.

@synaptiko
Copy link
Author

@rwwagner90 ok, I "found" a solution but I'm afraid you won't like it :-) It reads some properties available on app, app.project and tree which it's kind of hacky and I'm not sure if it's covering all the other possible cases.

Out of curiosity I also checked ember-cli-stylelint but it seems to me that it suffers by the same issue, it won't work inside the addon.

@synaptiko
Copy link
Author

Btw. by playing with this I discovered that even the code in master has this issue with duplicated output when your app depends on an addon with isDevelopingAddon() { return true; }.

@RobbieTheWagner
Copy link
Collaborator

@synaptiko I use ember-cli-stylelint in my addons and it works fine. I think people probably just shouldn't check in isDevelopingAddon() { return true; }

@synaptiko
Copy link
Author

We fixed it in our fork and later we revisit usage of ember-cli-stylelint. No need to keep this PR opened.

@synaptiko synaptiko closed this Nov 14, 2018
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

Successfully merging this pull request may close these issues.

Linting in addon
2 participants