Skip to content

Conversation

@hsubox76
Copy link
Contributor

  1. WebFrameworks in src/frameworks/index.ts was supposed to be imported from the new frameworks.ts file which uses imports (stable, bundleable) instead of runtime requires (causes problems in webpack bundle for VSCode plugin). James told me this was probably a bad rebase and it should have been checked in.

  2. A couple of places in the CLI code use require, but after being run through webpack, it replaces it with webpack's own require, which works slightly differently (mainly, it expects modules only, not files). There are 2 workarounds for this.

    1. Probably the most robust one is to use createRequire(import.meta.url) but unfortunately you can't use import.meta.url unless the target is changed from commonjs to node16 or nodenext, which I don't think the CLI codebase is ready for.
    2. The other workaround is to use a string replacer loader in webpack to replace specific appearances of require at compile time with __non_webpack_require__. TS hates this so we need a @ts-ignore, and we don't want to replace all instances of require (the webpack one works fine and probably better on actual modules), just specific ones.

@hsubox76 hsubox76 requested review from joehan and yuchenshi June 16, 2023 19:42
@hsubox76 hsubox76 marked this pull request as ready for review June 16, 2023 19:42
Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

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

I don't love string-replace-loader, as it feels a bit hacky + fragile, but I trust your judgement on whether it is the best choice here.

strict: true
},
{
search: "require(path",
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels quite fragile - consider at least adding some explicit comments inframeworks/utils.ts warning about this string replace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment.

@hsubox76
Copy link
Contributor Author

I don't love string-replace-loader, as it feels a bit hacky + fragile, but I trust your judgement on whether it is the best choice here.

I think the most robust choice is probably to use createRequire(import.meta.url) but it won't work with TypeScript until/if the CLI can use node16 or nodenext targets: https://stackoverflow.com/questions/73494747/ts1343-the-import-meta-meta-property-is-only-allowed-when-the-module-opti

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.03%. Comparing base (4612ef2) to head (46d9d23).
⚠️ Report is 1536 commits behind head on master.

Files with missing lines Patch % Lines
src/frameworks/utils.ts 0.00% 3 Missing ⚠️
src/dynamicImport.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6000      +/-   ##
==========================================
+ Coverage   54.94%   55.03%   +0.08%     
==========================================
  Files         342      339       -3     
  Lines       23264    23220      -44     
  Branches     4754     4746       -8     
==========================================
- Hits        12783    12779       -4     
+ Misses       9351     9311      -40     
  Partials     1130     1130              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hsubox76
Copy link
Contributor Author

I don't love string-replace-loader, as it feels a bit hacky + fragile, but I trust your judgement on whether it is the best choice here.

I think the most robust choice is probably to use createRequire(import.meta.url) but it won't work with TypeScript until/if the CLI can use node16 or nodenext targets: https://stackoverflow.com/questions/73494747/ts1343-the-import-meta-meta-property-is-only-allowed-when-the-module-opti

Actually I switched to using a conditional, it's not great either but it's less fragile than the string replace. It checks to see if __webpack_require__ is defined which if true, means it's in webpack, and it means non_webpack_require also exists, and it will sub it in for require. Strangely you can't check for typeof __non_webpack_require__ directly. Solution's described in this comment: webpack/webpack#4175 (comment)

Let me know what you think.

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.

4 participants