You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks a lot for the detailed write up @nathanmarks. The main priority I would have before merging this is ensuring that it is not going to cause any regressions in Prettier.
Please could you try applying the changes in your PR to Prettier to double check? From memory there is a fairly comprehensive set of tests for comments in JSX over there...
If this is a requirement for PRing, it may make more sense to create a CI build to test this scenario. If you decide this is a good idea, the dockerfile would look something like this (sorry if it's not 100%, copying from a personal project):
## Build a node application
from node:8.11.1
## typescript-eslint creation, build and test
WORKDIR /ts-eslint
COPY . .
RUN npm install --unsafe-perm
RUN npm run test
## prettier
RUN echo "### prettier"
WORKDIR /prettier
RUN git clone https://github.com/prettier/prettier .
RUN npm install --unsafe-perm
RUN npm link /ts-eslint
RUN npm run build
RUN npm run test
The text was updated successfully, but these errors were encountered:
I think that this was a unique situation.
A few months ago, a contributor added a seemingly innocuous change to the way JSX was parsed. This caused a bug which I believe meant that the comment detection was overly aggressive (it flagged urls within the JSX text as comments).
Another contributor then came in from the perspective of a prettier user, and put in a fix. This change caused a bug in which the comments were now ignored because they were not added to the AST (but this fixed the bugs from prettier's POV).
So with #703 we just wanted to make sure that this 3rd fix actually fixed everything on both sides of the fence.
Considering that there is a dependency on us from prettier, we should probably make sure that we don't break anything for them.
I don't see the problem with adding integration tests to ensure that we don't accidentally break them again.
Yes, exactly what Brad says regarding the context.
In terms of what to do long term, we could set up a nightly job for it, but actually making it part of the PR process would add too much overhead on our side I think.
I saw in PR #703 there was a request
If this is a requirement for PRing, it may make more sense to create a CI build to test this scenario. If you decide this is a good idea, the dockerfile would look something like this (sorry if it's not 100%, copying from a personal project):
The text was updated successfully, but these errors were encountered: