search

less / less.js

Less. The dynamic stylesheet language.
http://lesscss.org
Apache License 2.0
17k stars 3.41k forks source link

chore: use explicit path #3712

Open jimmywarting opened 2 years ago

jimmywarting commented 2 years ago

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:

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

matthew-dean commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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

matthew-dean commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/less/less.js/pull/3774