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

Skip assertDoc calls in production #3268

Merged
merged 4 commits into from Nov 16, 2017

Conversation

Projects
None yet
3 participants
@duailibe
Member

duailibe commented Nov 14, 2017

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

This comment has been minimized.

Member

suchipi commented Nov 14, 2017

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

This comment has been minimized.

Member

duailibe commented Nov 14, 2017

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

This comment has been minimized.

Member

suchipi commented Nov 14, 2017

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.

@duailibe duailibe changed the title from Skip redundant assertDoc calls in fill to Skip assertDoc calls in production Nov 14, 2017

throw new Error(
"Value " + JSON.stringify(val) + " is not a valid document"
);
if (process.env.NODE_ENV === "production") {

This comment has been minimized.

@suchipi

suchipi Nov 14, 2017

Member

Should be !==

This comment has been minimized.

@duailibe

duailibe Nov 15, 2017

Member

🙄 sorry about that

@duailibe

This comment has been minimized.

Member

duailibe commented Nov 15, 2017

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

@duailibe

This comment has been minimized.

Member

duailibe commented Nov 15, 2017

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!

@@ -2,7 +2,7 @@
function assertDoc(val) {
/* istanbul ignore if */
if (process.env.NODE_ENV === "production") {
if (process.env.NODE_ENV !== "production") {

This comment has been minimized.

@azz

azz Nov 15, 2017

Member

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

This comment has been minimized.

@duailibe

duailibe Nov 15, 2017

Member

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

This comment has been minimized.

@duailibe

duailibe Nov 15, 2017

Member

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

This comment has been minimized.

@azz

azz Nov 15, 2017

Member

Oh, right. Cool.

@suchipi

suchipi approved these changes Nov 15, 2017 edited

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

@duailibe duailibe merged commit 66d9b26 into prettier:master Nov 16, 2017

3 checks passed

codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 97.07% (+<.01%) compared to 2332c5c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@duailibe duailibe deleted the duailibe:fill-assert-doc branch Nov 16, 2017

@suchipi suchipi added this to the 1.9 milestone Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment