Skip to content

chore(gatsby): Create type declarations for gatsby plugin files #4928

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

Merged
merged 5 commits into from
Apr 14, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Apr 13, 2022

When creating a Gatsby plugin (which is what @sentry/gatsby actually is), there are certain files which need to live at the package root level. Because they are written in JS (not TS) and don't live in src/, we have thus far more or less ignored them as far as type-ful processing goes. With the recent updates to TS, jest, and ts-jest, typechecking has now gotten stricter (especially in tests) and so now declaration files are needed for those gatsby-required, root-level files if we want to test them (which we do).

This adds machinery to create, manage, and publish these declaration files.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 13, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.18 KB (+0.19% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 64.31 KB (-0.47% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.84 KB (-0.08% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 57.95 KB (-0.04% 🔽)
@sentry/browser - Webpack (gzipped + minified) 23.42 KB (+0.77% 🔺)
@sentry/browser - Webpack (minified) 81.22 KB (-0.6% 🔽)
@sentry/react - Webpack (gzipped + minified) 23.46 KB (+0.79% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.96 KB (-0.19% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.04 KB (-0.12% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.41 KB (-0.29% 🔽)

Comment on lines +13 to +14
!gatsby-browser.d.ts
!gatsby-node.d.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do we want to include these declaration files in the tarball? If yes, then we need to adjust the Gatsby-specific prepack script to copy them to build, too. Currently they do not end up in the tarball. Or is there another reason why we're adding them to .npmignore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to include these declaration files in the tarball?

I was on the fence about this. I opted for including them because they're useful for debugging, but I don't feel strongly about it. What do you think?

If yes, then we need to adjust the Gatsby-specific prepack script to copy them to build, too.

Oh, good catch. Unless you think it's a terrible idea to add them, I'll fix this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can't hurt to include them. Don't know enough about Gatsby to know how many people would need them but if we're already producing them why not. We have to copy them to build/types to be found by the types entry point, i think, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing just came to my mind: If, in the tarball, they end up in build/types, we could just adjust tsconfig.plugin.json to output them to build/types, right? Then we don't have to adjust Gatsby's prepack.ts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to copy them to build/types to be found by the types entry point, i think, right?

You know, this is an excellent question, to which I don't actually know the answer. ts-jest finds them where they are, and I'd have to experiment to see if it similarly finds them in types/. Another thought I have here, though, is that these two JS files are already outside of the entrypoint structure, so maybe it's fine that their declaration files are, too. Is the types endpoint even supposed to include them, given that main and module don't include the original source files?

Copy link
Member Author

@lobsterkatie lobsterkatie Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I added them to the prepack script (and a few other places I'd forgotten about), but left them alongside the the source files they describe. I say we try it like this, and if it's a problem, someone will tell us. (Though my gut says it's fine.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point, I didn't think about that. I say if it works for the source file this way, then why not for the declarations as well. Sounds good.

@lobsterkatie lobsterkatie force-pushed the kmclb-create-gatsby-type-files branch 3 times, most recently from 6f657d3 to c0591c6 Compare April 14, 2022 01:51
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@lobsterkatie lobsterkatie force-pushed the kmclb-create-gatsby-type-files branch from c0591c6 to 4b9f83d Compare April 14, 2022 13:45
@lobsterkatie lobsterkatie merged commit ffc80e2 into 7.x Apr 14, 2022
@lobsterkatie lobsterkatie deleted the kmclb-create-gatsby-type-files branch April 14, 2022 14:11
lobsterkatie added a commit that referenced this pull request Apr 14, 2022
This is a follow up to #4928, which added the creation of types files for the code in the gatsby SDK which lives outside of `src/`. One change which was missed was to have GHA include these new files in the build cache, so that they are available when tests are run. This fixes that oversight.
Lms24 pushed a commit that referenced this pull request Apr 26, 2022
When creating a Gatsby plugin (which is what `@sentry/gatsby` actually is), there are certain files[1] which need to live at the package root level. Because they are written in JS (not TS) and don't live in `src/`, we have thus far more or less ignored them as far as type-ful processing goes. With the recent updates to TS, jest, and ts-jest, typechecking has now gotten stricter (especially in tests) and so now declaration files are needed for those gatsby-required, root-level files if we want to test them (which we do).

This adds machinery to create, manage, and publish these declaration files.

[1] https://www.gatsbyjs.com/docs/files-gatsby-looks-for-in-a-plugin/
Lms24 pushed a commit that referenced this pull request Apr 26, 2022
This is a follow up to #4928, which added the creation of types files for the code in the gatsby SDK which lives outside of `src/`. One change which was missed was to have GHA include these new files in the build cache, so that they are available when tests are run. This fixes that oversight.
lobsterkatie added a commit that referenced this pull request Apr 26, 2022
When creating a Gatsby plugin (which is what `@sentry/gatsby` actually is), there are certain files[1] which need to live at the package root level. Because they are written in JS (not TS) and don't live in `src/`, we have thus far more or less ignored them as far as type-ful processing goes. With the recent updates to TS, jest, and ts-jest, typechecking has now gotten stricter (especially in tests) and so now declaration files are needed for those gatsby-required, root-level files if we want to test them (which we do).

This adds machinery to create, manage, and publish these declaration files.

[1] https://www.gatsbyjs.com/docs/files-gatsby-looks-for-in-a-plugin/
lobsterkatie added a commit that referenced this pull request Apr 26, 2022
This is a follow up to #4928, which added the creation of types files for the code in the gatsby SDK which lives outside of `src/`. One change which was missed was to have GHA include these new files in the build cache, so that they are available when tests are run. This fixes that oversight.
lobsterkatie added a commit that referenced this pull request Apr 26, 2022
When creating a Gatsby plugin (which is what `@sentry/gatsby` actually is), there are certain files[1] which need to live at the package root level. Because they are written in JS (not TS) and don't live in `src/`, we have thus far more or less ignored them as far as type-ful processing goes. With the recent updates to TS, jest, and ts-jest, typechecking has now gotten stricter (especially in tests) and so now declaration files are needed for those gatsby-required, root-level files if we want to test them (which we do).

This adds machinery to create, manage, and publish these declaration files.

[1] https://www.gatsbyjs.com/docs/files-gatsby-looks-for-in-a-plugin/
lobsterkatie added a commit that referenced this pull request Apr 26, 2022
This is a follow up to #4928, which added the creation of types files for the code in the gatsby SDK which lives outside of `src/`. One change which was missed was to have GHA include these new files in the build cache, so that they are available when tests are run. This fixes that oversight.
lobsterkatie added a commit that referenced this pull request Apr 29, 2022
This adds a check to the beginning of the gatsby tests which makes sure that the necessary types files exist and creates them before starting the tests if they don't. 

Note: These files are not the normal files built by `yarn build:types` but additional ones made necessary by the structure gatsby plugins are required to have. See #4928.
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
When creating a Gatsby plugin (which is what `@sentry/gatsby` actually is), there are certain files[1] which need to live at the package root level. Because they are written in JS (not TS) and don't live in `src/`, we have thus far more or less ignored them as far as type-ful processing goes. With the recent updates to TS, jest, and ts-jest, typechecking has now gotten stricter (especially in tests) and so now declaration files are needed for those gatsby-required, root-level files if we want to test them (which we do).

This adds machinery to create, manage, and publish these declaration files.

[1] https://www.gatsbyjs.com/docs/files-gatsby-looks-for-in-a-plugin/
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
This is a follow up to #4928, which added the creation of types files for the code in the gatsby SDK which lives outside of `src/`. One change which was missed was to have GHA include these new files in the build cache, so that they are available when tests are run. This fixes that oversight.
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
This adds a check to the beginning of the gatsby tests which makes sure that the necessary types files exist and creates them before starting the tests if they don't. 

Note: These files are not the normal files built by `yarn build:types` but additional ones made necessary by the structure gatsby plugins are required to have. See #4928.
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.

2 participants