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

Fix transpiled code loading #71

Merged
merged 6 commits into from Mar 21, 2022
Merged

Conversation

castarco
Copy link
Contributor

@castarco castarco commented Mar 18, 2022

Changes

  • Improve compatibility to load transpiled code into worker threads
  • Make it possible to directly load .ts files into worker threads when using ts-node (except in Windows).

Not covered in this PR

Some potential improvements had to be left out of this PR to contain its scope:

  • Directly loading .ts files into worker threads when running the code in Windows platforms.
  • Support for TSM modules loader ( https://github.com/lukeed/tsm )
  • "native" ESM support for not-transpiled .ts files.

Extra comments

It seems that the coverage metrics have gone down mainly because of leaving out tests on Windows for the direct loading of .ts files without transpilation. I'm not sure if I should explicitly document this lack of support, or if it's already enough stating it in the PR itself, as it's "just" a corner case.

Related

@coveralls
Copy link

coveralls commented Mar 18, 2022

Pull Request Test Coverage Report for Build 2016805012

  • 6 of 10 (60.0%) changed or added relevant lines in 1 file are covered.
  • 123 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-38.4%) to 47.87%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/worker.js 6 10 60.0%
Files with Coverage Reduction New Missed Lines %
lib/worker.js 15 55.17%
lib/wait.js 24 15.69%
index.js 84 50.28%
Totals Coverage Status
Change from base Build 2014932325: -38.4%
Covered Lines: 181
Relevant Lines: 343

💛 - Coveralls

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work! Can you add a tsconfig targeting esnext?

test/ts/to-file.es5.cjs Outdated Show resolved Hide resolved
test/ts/to-file.es6.cjs Outdated Show resolved Hide resolved
test/ts/to-file.es6.mjs Outdated Show resolved Hide resolved
@castarco
Copy link
Contributor Author

Any idea on why the tests might be flaky? I see that they are not failing in a consistent manner.

@mcollina
Copy link
Member

There are likely some bugs in some parts of the code. As long as we can get a clean / full green CI run we are good.

@castarco
Copy link
Contributor Author

castarco commented Mar 21, 2022

Update:

  • tsc doesn't play well with yarn-pnp, I'm looking into that: ✅
  • There are other problems in Windows, related with the temporary files management (this only happens for the new TS support). Also looking into that.

@castarco castarco marked this pull request as ready for review March 21, 2022 14:34
See pinojs/pino#1243 (comment)

Added test confirming that there is a real issue, and that it is
corrected.
The added type declarations have been added for testing purposes, they
need more love to be useful for package users.
- cover esnext code generation
- use bash shell for tests in windows
- wrap transpile script execution
- use yarn as runner when testing yarn-pnp
Copy link
Contributor

@ramonmulia ramonmulia left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit d048320 into pinojs:main Mar 21, 2022
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.

None yet

4 participants