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

build: modify ts-ignore for /test & /src #3036

Merged
merged 1 commit into from
Oct 31, 2022
Merged

build: modify ts-ignore for /test & /src #3036

merged 1 commit into from
Oct 31, 2022

Conversation

VerteDinde
Copy link
Member

Modifies the generated ts-ignores to ignore /test and /src

@VerteDinde VerteDinde requested a review from a team October 31, 2022 23:33
@VerteDinde VerteDinde merged commit 533f56c into main Oct 31, 2022
@VerteDinde VerteDinde deleted the modify-tsignore branch October 31, 2022 23:37
@erikian
Copy link
Member

erikian commented Nov 1, 2022

@VerteDinde maybe I'm missing something here, but I believe it's still necessary to explicitly ignore *.ts, *.map and tsconfig.json in addition to /src and /test.

How I'm testing:

mkdir test
cd test
npm init -y
yarn add --dev @electron-forge/cli

Then get the .tgz URL for @electron-forge/cli@^6.0.0-beta.73 from yarn.lock (it's https://registry.yarnpkg.com/@electron-forge/cli/-/cli-6.0.0-beta.73.tgz for me). Open the file and take a look around:
image

image

If you do the same with beta-71 (https://registry.yarnpkg.com/@electron-forge/cli/-/cli-6.0.0-beta.71.tgz), which didn't include this commit, the .map files and tsconfig.json won't be there. Unless there's some use case for them in the distributed packages, they should probably be ignored as before.

(On a side note, .eslintrc.json is still getting through as shown in the first screenshot 😢)

@MarshallOfSound
Copy link
Member

@erikian The .map files are useful as node will translate stack traces using them (which makes debugging way easier).

The .ts and tsconfig.json files were removed from this list because our templates legitimately need to publish files with those names. We should be more surgical about these things and add a files property to the package.json of each module (much safer that way IMO)

@erikian
Copy link
Member

erikian commented Nov 1, 2022

Got it. +1 on having a files property in every module, I can open a PR for that if it's something you guys would like some help with 😃

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.

4 participants