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

Converts package to publishing only .js/.d.ts files #38

Merged
merged 2 commits into from
May 10, 2021

Conversation

scalvert
Copy link
Contributor

The motivation behind this change is to adjust the published output of this package to include only .js/.d.ts files instead of .ts. files. The inclusion of .ts files in the published output results in tools that attempt dynamic resolution of commonjs published modules (ts-node, ts-jest) become confused by the presence of .ts files, when the source project itself is a .ts project.

Example:

Source Project (TypeScript project with dynamic ts loader like ts-jest)

  • node_modules
    • some-transitive-dep
      • resolve-package-path

In the case of the above, "Source Project" attempts to run tests via ts-jest, which resolves the package but detects the presence of a .ts file in the published output, which results in the following error:

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/en/ecmascript-modules for how to enable it.
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    node_modules/some-transitive-dep/node_modules/resolve-package-path/lib/resolve-package-path.ts:5
    import fs = require('fs');
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1350:14)
      at Object.<anonymous> (node_modules/some-transitive-dep/node_modules/resolve-package-path/index.js:3:32)

Deleting the .ts files from the published package resolves the issue.

@stefanpenner happy to discuss in person if needed.

@stefanpenner
Copy link
Owner

stefanpenner commented May 10, 2021

looks good, let's include the map files as well. THANK YOU!

@scalvert scalvert marked this pull request as draft May 10, 2021 22:14
@stefanpenner
Copy link
Owner

I believe both of the following flags may be needed, in-order to to achieve your goal + have good maps.

"sourceMap": true,
"inlineSources": true,

@scalvert
Copy link
Contributor Author

Done. Thanks, Stef!

@scalvert scalvert marked this pull request as ready for review May 10, 2021 22:39
@scalvert
Copy link
Contributor Author

Made a few extra tweaks to linting config to make the eslint gods happy. Let me know if you see anything else odd.

BTW, I opted not to convert everything to import/export syntax, mainly because there's structure that doesn't lend well to converting to import/export without a bit of refactoring. I wanted to change as little as possible, so opted to leave that alone for now.

Also of note, I removed Node 10 support since that's EOL. We should rev a major on this.

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