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

Remove interface usage from types package for Node #451

Merged
merged 1 commit into from Oct 13, 2021

Conversation

luismiramirez
Copy link
Member

@luismiramirez luismiramirez commented Oct 1, 2021

Remove interface usage from types package for Node

All Node.js-specific interfaces from the types package are now defined
inside the nodejs core package. There's still a dependency from types
package for common types as Func, HashMap, and HasMapValue.

With this change, we keep taking advantage of interfaces, but now they're
defined in a place where they're used.

@luismiramirez luismiramirez force-pushed the remove-types-package-dependency branch 6 times, most recently from 2049166 to d8804a4 Compare October 4, 2021 13:43
@luismiramirez luismiramirez marked this pull request as ready for review October 4, 2021 13:43
@backlog-helper

This comment has been minimized.

package-lock.json Outdated Show resolved Hide resolved
packages/koa/src/patches/index.ts Outdated Show resolved Hide resolved
packages/nodejs/package-lock.json Outdated Show resolved Hide resolved
@backlog-helper

This comment has been minimized.

All Node.js specific interfaces from the types package are now defined
inside nodejs core package. There's still a dependency from types
package for common types as `Func`, `HashMap`, and `HasMapValue`.

With this change we keep taking advantage of interfaces but now they're
defined in a place where they're used.
@luismiramirez luismiramirez force-pushed the remove-types-package-dependency branch from d8804a4 to 9c8319e Compare October 6, 2021 07:45
@backlog-helper
Copy link

backlog-helper bot commented Oct 7, 2021

While performing the daily checks some issues were found with this Pull Request.


New issue guide | Backlog management | Rules | Feedback

@luismiramirez luismiramirez merged commit faa2d3b into main Oct 13, 2021
@luismiramirez luismiramirez deleted the remove-types-package-dependency branch October 13, 2021 07:13
tombruijn added a commit that referenced this pull request Feb 1, 2022
We changed the types used in PR #451. We no longer rely on the
`@appsignal/types` package for this, but define it in the package
itself.

The ScopeManager appears to not have been updated to match and is the
only place in which the `NodeSpan` from the types package is still
used. If we update this usage we can remove all Node.js types from the
types package.
tombruijn added a commit that referenced this pull request Feb 1, 2022
We changed the types used in PR #451. We no longer rely on the
`@appsignal/types` package for this, but define it in the package
itself.

The ScopeManager appears to not have been updated to match and is the
only place in which the `NodeSpan` from the types package is still
used. If we update this usage we can remove all Node.js types from the
types package.
tombruijn added a commit to appsignal/appsignal-javascript that referenced this pull request Feb 3, 2022
We moved all types to the Node.js package in PRs:

- appsignal/appsignal-nodejs#451
- appsignal/appsignal-nodejs#587

Let's remove them from the types package to avoid confusion. People can
instead use the types/interfaces the Node.js package exports if need.
tombruijn added a commit to appsignal/appsignal-javascript that referenced this pull request Feb 3, 2022
We moved all types to the Node.js package in PRs:

- appsignal/appsignal-nodejs#451
- appsignal/appsignal-nodejs#587

Let's remove them from the types package to avoid confusion. People can
instead use the types/interfaces the Node.js package exports if need.
tombruijn added a commit to appsignal/appsignal-javascript that referenced this pull request Feb 3, 2022
We moved all types to the Node.js package in PRs:

- appsignal/appsignal-nodejs#451
- appsignal/appsignal-nodejs#587

Let's remove them from the types package to avoid confusion. People can
instead use the types/interfaces the Node.js package exports if need.
tombruijn added a commit that referenced this pull request Feb 10, 2022
Update the types package to the version without Node.js types. They're
all part of the Node.js package itself now since PR #451.

This avoids confusion about the availability of Node.js types in the
types package.

Fixes #590
tombruijn added a commit that referenced this pull request Feb 11, 2022
Update the types package to the version without Node.js types. They're
all part of the Node.js package itself now since PR #451.

This avoids confusion about the availability of Node.js types in the
types package.

Fixes #590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants