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(conf): lazy load cosmiconfig's TypeScriptLoader #1403

Merged

Conversation

timofei-iatsenko
Copy link
Collaborator

Description

This PR is attempt to fix: #1401

cosmiconfig-typescript-loader written in the way that it doesn't matter do you really load a Typescript file in config or not, once this loader is added to cosmiconfig it requires ts-node and typescript. This is indeed unwanted behavior because some of the users may not have them and may not want to use them at all.

This PR changes 2 things:

  1. Lazy load the loader, once cosmiconfig discovered a config with ts extension
  2. Add peerDependecies on typescript and ts-node for the @lingui/conf package. And also i marked them as optional.

Why this happened? Why our test didn't catch this?

It's because node modules resolution system works like that.
Take 'examples/create-react-app' as an example. It has it's own pacakge.json and own node_modules.
Furthermore, it doesn't list ts-node in this file, and indeed node_modules folder doesn't have this dependency. So why this project doesn't fall with that change?

It's because we have another node_modules folder on the root level of monorepo and node found this module there.

  • root
    • node_modules <-- from here
    • examples
      • create-react-app
        • node_modules
        • package.json

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Fixes # (issue)

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Feb 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 3, 2023 at 7:09PM (UTC)

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

size-limit report 📦

Path Size
./packages/core/build/esm/index.js 1.76 KB (0%)
./packages/detect-locale/build/esm/index.js 812 B (0%)
./packages/react/build/esm/index.js 1.78 KB (0%)
./packages/remote-loader/build/esm/index.js 7.29 KB (0%)

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 82.80% // Head: 82.79% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (d30f5ee) compared to base (a8c110d).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1403      +/-   ##
==========================================
- Coverage   82.80%   82.79%   -0.01%     
==========================================
  Files          66       66              
  Lines        1756     1761       +5     
  Branches      487      488       +1     
==========================================
+ Hits         1454     1458       +4     
  Misses        174      174              
- Partials      128      129       +1     
Impacted Files Coverage Δ
packages/conf/src/index.ts 84.81% <80.00%> (-0.33%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrii-bodnar
Copy link
Contributor

Have no opinion on that but it looks ok if it solves the problem. @thekip did you manage to reproduce it?

@niksauer @ryanb can you please check if this fix works in your cases?

@ryanb
Copy link

ryanb commented Feb 4, 2023

@thekip thanks for your work on this.

@andrii-bodnar I've tried adding the @lingui/conf package pointing to this branch using GitPkg

yarn add 'https://gitpkg.now.sh/thekip/js-lingui/packages/conf?fix/typescript-cosmiconfig'

This appears to work on my local, however my deployment pipeline results in this error.

/app/release/20230204001656/node_modules/@lingui/conf/index.js:1
export * from "./src"
^^^^^^

SyntaxError: Unexpected token 'export'
    at wrapSafe (internal/modules/cjs/loader.js:1001:16)
    at Module._compile (internal/modules/cjs/loader.js:1049:27)
    ...

Perhaps I'm not installing the package correctly? Is there an alternative way to install the package from this branch?

I only saw the ts-node error during deployment as well. I have yet to determined why it's only showing up during deployment.

For the record, I'm using Webpack to bundle everything and am not using TypeScript or React in this project.

@timofei-iatsenko
Copy link
Collaborator Author

@ryanb please follow a contribution guide. You need to build package and upload to Verdaccio, then you would be able to install it as usually. Of course on the ci that would not work because verdaccio would not be available outside your machine.

@niksauer
Copy link

niksauer commented Feb 5, 2023

Can confirm this fix is working, having followed the contribution guide. Thanks!

@andrii-bodnar andrii-bodnar merged commit 617a333 into lingui:main Feb 6, 2023
@timofei-iatsenko timofei-iatsenko deleted the fix/typescript-cosmiconfig branch February 6, 2023 10:17
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.

Cannot find module 'ts-node'
5 participants