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

chore: use explicit path #3712

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

Conversation

jimmywarting
Copy link

@jimmywarting jimmywarting commented Apr 15, 2022

What:

I changed some imports to be explicit (regarding ESM rules)
Remote sources can't just guess what file you meant to import... things such as index.js is bad...

Why:

File extension is mandatory

How:

my vscode settings automatically trims whitespace, so those got included as well...

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

there are a few places left... but thought i do little by little

@jimmywarting jimmywarting changed the title use explicit path chore: use explicit path Apr 15, 2022
@matthew-dean
Copy link
Member

I'm confused about this. I don't see a Github issue or anything that provides context as to why this is necessary other than developer preference.

@jimmywarting
Copy link
Author

jimmywarting commented Apr 16, 2022

I for instances would like to be able to use the original code rather than some down leveled compiled version
also, some places where already using .js

@jimmywarting
Copy link
Author

jimmywarting commented Apr 16, 2022

From node's docs:

Mandatory file extensions#
A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

This behavior matches how import behaves in browser environments, assuming a typically configured server.

so, no... it's not just a "developer preference".

@matthew-dean
Copy link
Member

The source code right now is meant to be transpiled. Although I think it would be great if we moved back to a non-transpiled model, based on current supported Node versions. (The problem in the past is that there were breaking changes for downstream dependents that expected functions / function prototypes vs. classes.)

But in the short term, I would be hesitant to make this change without some kind of exploration of side-effects.

@jimmywarting
Copy link
Author

can this be merged?
or would you rather have wanted me to send smaller changes / PR's?

@matthew-dean
Copy link
Member

No, I don't see how this can be merged as there are no tests proving this doesn't break in any Node environment when imported directly.

@jimmywarting
Copy link
Author

there are no tests proving this doesn't break in any Node environment when imported directly.

It still dose very much depends on compiling the source and consumers do still importing the build version.
as there are not yet any place where it imports the original (uncompiled) source directly.

in any case. how do you propose we go about it and write a test for this?
i don't believe just adding .js will "break" things

@iChenLei
Copy link
Member

iChenLei commented Jan 16, 2023

It appears that the e2e test has failed, and it's not due to this PR. I will attempt to fix the CI. My pr for CI fix #3774

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.

3 participants