Skip assertDoc calls in production

[duailibe]

Context: #3263 (comment)

When printDocToString calls fill, it's not necessary to call assertDoc for all of the remaining elements since they already checked when the doc was originally created (somewhere in printAstToDoc).

[suchipi]

If we never assertDoc in fill, are we still covered? Or are there situations where we call fill without asserting doc in printAstToDoc?

We might want to offload this kind of check to a type checker like flow or etc instead of taking the runtime hit (static type checking instead). But of course, adding flow could make things more overwhelming for new contributors. Not sure how often new contributors make changes to the doc printer though (I suspect rarely).

[duailibe]

If we never assertDoc in fill, are we still covered? Or are there situations where we call fill without asserting doc in printAstToDoc?

I'm not sure I understand this correctly. But if I did, I think we're not covered, since the case for fill is to receive values (not other docs: group or concat): fill(["text", separator, "line", separator, ...]) so those would be "unchecked" docs.

I was wondering what would be the downside of skipping the assertDoc check entirely in production. Our own code is covered by tests and the only problem that could happen would be a parser misbehaving (like returning something not string when it should be a string?).

[suchipi]

Sorry that I wasn't clear, but yes, you understood correctly.

I like the idea of skipping assertDoc in production. Let's do that instead of special-casing fill.

[suchipi]

Should be !==

[duailibe]

🙄 sorry about that

[duailibe]

@suchipi Looks like just calling the function is expensive enough :( (or the forEach call)

[duailibe]

I had to wrap each call with the if and add the replace in rollup build script to remove them from the dist file (optional but skips an if check, which is a good thing). I thought it would not get removed without minification but it was, to my surprise.

It's a bit ugly that bunch of ifs lying around but I think it's worth the benefit. We could change them with a __DEV__ call witch is a bit fancier, and we just have to change the rollup replace plugin to achieve that. Another option would be automatically removing them in rollup but I thought it was too much magic.

Let me know what you think!

[azz]

Not sure this is the right way of doing it, a lot of people will run prettier in "development" env.

[duailibe]

@azz I updated the PR, can you check again?

[duailibe]

To clarify, when building the bundle, we will replace that check with "production" to remove that entirely

[azz]

Oh, right. Cool.

[suchipi]

I don't mind the ifs; I think people are pretty used to seeing process.env.NODE_ENV in code these days, compared to something more "magic" like __DEV__.