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

Improve ESM support by using import with file extensions #302

Open
wants to merge 73 commits into
base: master
Choose a base branch
from

Conversation

JumpLink
Copy link

@JumpLink JumpLink commented Jul 12, 2022

Summary of changes

Add .js file extension to all internal imports.

This fixes #161 (see latest comments).

For example, the error message without this fix looks like this:

node_modules/@deepkit/app/dist/esm/src/service-container.d.ts:3:63 - error TS2835: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './module.js'?

3 import { AppModule, MiddlewareConfig, ModuleDefinition } from './module';

By the way

I used the regex search in VSCode to find all internal imports without a file extension with this search term:

(import|from) ("|')\..*(?<!.js)("|')

This worked very well, we could also build a small lint script with this regular expression to check if there are imports without file extension.

Relinquishment of Rights

Please mark following checkbox to confirm that you relinquish all rights of your changes:

  • I waive and relinquish all rights regarding this changes (including code, text, and images) to Deepkit UG (limited), Germany. This changes (including code, text, and images) are under MIT license without name attribution, copyright notice, and permission notice requirement.

@marcj
Copy link
Member

marcj commented Jul 12, 2022

That looks vey cool, thank you @JumpLink! If tests are green, I'm going to merge that.

This worked very well, we could also build a small lint script with this regular expression to check if there are imports without file extension.

Yes, good idea, let's do that.

@marcj
Copy link
Member

marcj commented Jul 12, 2022

I guess we have to exclude files in /tests/* + frontend packages framework-debug-ui, api-console-gui, and orm-browser-gui

@JumpLink
Copy link
Author

@marcj Can you the workflow?

@marcj
Copy link
Member

marcj commented Jul 12, 2022

@JumpLink what do you mean?

@JumpLink
Copy link
Author

@marcj "1 workflow awaiting approval "

@marcj
Copy link
Member

marcj commented Jul 12, 2022

oh, I'm sorry🤣 approved

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #302 (4cd70d1) into master (b45ede8) will decrease coverage by 1.36%.
The diff coverage is 69.17%.

@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
- Coverage   78.31%   76.94%   -1.37%     
==========================================
  Files         169      122      -47     
  Lines       17672    14716    -2956     
  Branches     4615     3973     -642     
==========================================
- Hits        13839    11323    -2516     
+ Misses       3833     3393     -440     
Impacted Files Coverage Δ
packages/core/src/path.ts 0.00% <0.00%> (ø)
packages/core/src/runtime.ts 0.00% <0.00%> (ø)
packages/http/src/logger.ts 0.00% <0.00%> (ø)
packages/http/src/static-serving.ts 0.00% <0.00%> (ø)
packages/http/src/http.ts 80.91% <20.00%> (-0.84%) ⬇️
packages/broker/src/client.ts 92.02% <100.00%> (ø)
packages/broker/src/kernel.ts 87.56% <100.00%> (ø)
packages/bson/src/bson-deserializer-templates.ts 95.65% <100.00%> (ø)
packages/bson/src/bson-deserializer.ts 96.00% <100.00%> (ø)
packages/bson/src/bson-parser.ts 71.61% <100.00%> (ø)
... and 74 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@JumpLink
Copy link
Author

JumpLink commented Jul 12, 2022

@marcj I find it difficult to test my changes locally, so I am not sure if the gui packages are working.

E.g. When I tried to install the package resolve-typescript-plugin in @deepkit/framework-debug-gui NPM has told me that "@deepkit/ui-library": "^1.0.1-alpha.56" cannot be found on the NPM registry. So I did it by hand to see if the test goes through, but this one doesn't seem to test the gui packages?

@JumpLink
Copy link
Author

@marcj I will be unavailable for a week now, but I can finish the PR after that, maybe you can collect feedback for me until then. I will then also try again to be able to test things locally.

@marcj
Copy link
Member

marcj commented Jul 14, 2022

E.g. When I tried to install the package resolve-typescript-plugin in @deepkit/framework-debug-gui NPM has told me that "@deepkit/ui-library": "^1.0.1-alpha.56" cannot be found on the NPM registry. So I did it by hand to see if the test goes through, but this one doesn't seem to test the gui packages?

Please see the notes in DEVELOPMENT.md in the root folder:

  • When one of the package.json files is modified (adding, removing or updating dependencies) you will need to re-run the npm bootstrap and npm link commands from above
  • Never install NPM dependencies directly inside of any of the packages in the packages/* directory.

@marcj
Copy link
Member

marcj commented Jul 14, 2022

@marcj I will be unavailable for a week now, but I can finish the PR after that, maybe you can collect feedback for me until then. I will then also try again to be able to test things locally.

ok, cool! Let me know if something doesn't work. I'm available 24/7 in Discord. Much easier there to solve such issues 👍

@JumpLink JumpLink mentioned this pull request Jul 18, 2022
1 task
@JumpLink
Copy link
Author

@marcj Can you rerun the workflow and then merge this PR?

@H4ad
Copy link

H4ad commented Aug 1, 2022

Do you have any plans to merge this PR? I want to add deepkit support to my library, but I faced some errors when creating tests because each dependency makes me add another dependency as peer dependency so I don't get Cannot find module error.

And from what I read, this is the solution to my problem hahaha

@JumpLink
Copy link
Author

JumpLink commented Aug 8, 2022

Hi @H4ad,
@marcj plans to help me to resolve the latest issues of this PR and then plans to merge the PR.

@NexZhu
Copy link
Contributor

NexZhu commented Sep 11, 2022

Hi, is there anyhing I can help to make this merged faster? This is blocking my project from converting to ESM.

@JumpLink
Copy link
Author

@NexZhu maybe you can Help me top fix some Tests in the Branche of this PR

@NexZhu
Copy link
Contributor

NexZhu commented Sep 22, 2022

Sorry, being very busy lately, will help when I'm able to

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.

Module resolution errors
5 participants