search

mantrajs / mantra-cli

Command line interface for building Meteor apps with Mantra
MIT License
135 stars 34 forks source link

Don't update index.js if file exists #99

Closed thancock20 closed 7 years ago

thancock20 commented 7 years ago

Fixes #52

sungwoncho commented 7 years ago

There is a subtle difference between the original issue and the issue this PR is addressing.

This PR simply prevents duplicate definition in index.js. But I think a more fundamental solution would be to prevent calling updateIndexFile if the file to be generated even exists.

Currently we check the existence of the file here.

One solution I think we can pursue is to return {exists: Boolean, outputPath: String} from _generate function (see signature). Then we can use the return value to conditionally call updateIndexFile here.

What do you think @thancock20?

thancock20 commented 7 years ago

Yes, that's definitely doable. There would need to be conditional calls in the generators for method, publication, collection, and action. Correct?

Also, what's the likelihood of the edge case where the file exists, but the lines have been deleted from the index?

sungwoncho commented 7 years ago

@thancock20 You are correct. We will need conditionals in all generators that updated index.js.

Has that edge case occurred to you often? I personally think we should fix the cause of that edge case rather than coding defensively around it.

Sadly I have seen some cases where index.js is not updated correctly. Maybe that could be a separate issue on its own.

thancock20 commented 7 years ago

I've never had that edge case myself. I just have a habit of having weird edge cases pop into my head sometimes, when I'm planning out a solution.

I've got the changes made now, though the function signature for _generate should be changed as well. I'm guessing @return {Object}, but I'm not sure what to put for the description.

Actually, I was surprised that putting the outputPath in an object didn't break anything. But apparently it isn't actually used anywhere?

sungwoncho commented 7 years ago

Thanks for this. Yeah it seems that outputPath is not being used anywhere. We could refactor by simplifying the return value.