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

add absolute file prefix for dynamic imports #815

Merged
merged 4 commits into from
Jun 20, 2022

Conversation

robertkiel
Copy link
Contributor

@robertkiel robertkiel commented Jun 19, 2022

Re #812

Main change

Always add the file:// prefix when loading ES moduls or CommonJS scripts.

Tested with:

How to test

npm run bootstrap
cd packages/express-openapi
npm run test

@jsdevel jsdevel merged commit c34f542 into kogosoftwarellc:master Jun 20, 2022
@jsdevel
Copy link
Contributor

jsdevel commented Jun 20, 2022

thank you @robertkiel !

@7mllm7
Copy link

7mllm7 commented Jul 31, 2022

@robertkiel care you to explain the reason for this change?
This specific line basically fails my build (with webpack, when trying to bundle openapi-framework in a single bundle...).

I get:

ERROR in ./node_modules/openapi-framework/dist/index.js 132:47-84
Module not found: Error: Can't resolve 'file:/' in '/............./myproject/node_modules/openapi-framework/dist'

Which comes from exactly this line:

const imported = await import(`file://${fsRoutesItem.path}`);

If I remove the file:// bit, it's alright, but since you actually purposely added this, I don't want to change it.

Thanks!

@robertkiel
Copy link
Contributor Author

@robertkiel care you to explain the reason for this change?
This specific line basically fails my build (with webpack, when trying to bundle openapi-framework in a single bundle...).

I get:

ERROR in ./node_modules/openapi-framework/dist/index.js 132:47-84
Module not found: Error: Can't resolve 'file:/' in '/............./myproject/node_modules/openapi-framework/dist'

Which comes from exactly this line:

const imported = await import(`file://${fsRoutesItem.path}`);

If I remove the file:// bit, it's alright, but since you actually purposely added this, I don't want to change it.

Thanks!

The Windows version of Node.js requires absolute paths when loading ES modules - in contrast to Linux-based OSes and MacOS which also accept a relative path.

At this point, it could be worth to have a special treatment for Windows-based machines.

Which machine and which version of Node.js are you running?

@7mllm7
Copy link

7mllm7 commented Aug 1, 2022

@robertkiel thanks for your reply.
I'm running macOS Monterey (12.4) and Node.js v14.19.2.

Any idea how I can avoid this error without forking 🤔?

Thanks!

@robertkiel
Copy link
Contributor Author

Probably upgrade to Node.js 16

@coryasilva
Copy link

This should have been handled as @7mllm7 suggested.

This change is breaking our jest tests.

Node 20
Mac local dev
Linux container
```

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