sublimelsp / LSP-tailwindcss

Tailwind css support for Sublime's LSP plugin
MIT License
54 stars 5 forks source link

resolve the tailwindcss path based on the open file path #30

Closed predragnikolic closed 2 years ago

predragnikolic commented 2 years ago

closes #29

rchl commented 2 years ago

It really doesn't feel right to do that. The user should instead ensure that he's opening the root folder. This will just produce random results depending on what file is opened first.

Many servers require specific structure and i don't think we should be trying to fixup things like that.

predragnikolic commented 2 years ago

Each time you write a comment, it forces me to start questioning my actions :D

Reasons why I would consider merging this:

  1. For the user it would be better if this works without them having to care if it is the root folder.
  2. The tailwindcss extension for VS code support starting the tailwindcss ls even when the node_modules is not at the root.

Reasons why I would against merging this:

This will just produce random results depending on what file is opened first.

Yes, I have the same concern.

predragnikolic commented 2 years ago

This will just produce random results depending on what file is opened first.

What if the user opens a ts file in the root folder, but the node_modules folder is nested somewhere bellow".... so yeah... we will still have this problem...

predragnikolic commented 2 years ago

This PR is not the correct fix.

davidwebca commented 2 years ago

Mmm I'm not sure I understand the logic here. If a file is opened at the root and the node_modules folder is nested below, node_modules nested below shouldn't affect what's in their parent anyway from the get go. Isn't that what's implied all the time for node / frontend development? node_modules folder affects its peers and children, but not files located in the parent folders usually... 🤔

So locating tailwind according to the current opened file works, but you also have to find a the config file too, no? 🤔

Maybe I don't understand the logic. What would be the unpredictable results?

rchl commented 2 years ago

It looks like the server gets out of it way to find the tailwind config file and also node_modules so then I guess it would be OK to make this change.

With a project structure like:

.
├── README.md
├── aureliajs
│   ├── aurelia_project
│   ├── cypress
│   ├── cypress.config.js
│   ├── cypress.json
│   ├── favicon.ico
│   ├── index.html
│   ├── karma.conf.js
│   ├── node_modules
│   ├── package-lock.json
│   ├── package.json
│   ├── src
│   ├── tailwind.config.js
│   ├── tailwind.css
│   ├── tailwindcss-plugins
│   ├── test
│   └── tsconfig.json
├── editor.sublime-project
└── test.css

and opening test.css in the root it appears to be able to both find the config file and tailwindcss from node_modules:

LSP-tailwindcss: Found Tailwind CSS config file: /Users/../lsp-tailwindcss-client-error/aureliajs/tailwind.config.js
LSP-tailwindcss: Loaded postcss v8.4.5: /Users/../lsp-tailwindcss-client-error/aureliajs/node_modules/postcss
LSP-tailwindcss: Loaded tailwindcss v3.0.2: /Users/../lsp-tailwindcss-client-error/aureliajs/node_modules/tailwindcss
predragnikolic commented 2 years ago

What would be the unpredictable results?

@davidwebca given the following folder structure:

.
├── nested_folder
│   ├── index1.html
│   ├── node_modules
│   ├── package.json
│   └── tailwind.config.js
└── index2.html

With my first fix. There are two cases:

  1. someone can first open the index1.html - in this case the the resolve script will be able to find the tailwindcss dependecy and the server will be started.
  2. someone can first open the index2.html - in this case the the resolve script wont be able to find the tailwindcss dependecy and the server wont be started.
predragnikolic commented 2 years ago

Can this be merged?

BTW I was also thinking, If someone still reports issues like this, I will consider removing the is_allowed_to_start entirely and always start the LSP-tailwindcss server. The server would be https://github.com/sublimelsp/LSP-tailwindcss/blob/5c91856f2b255cd96252ddf92da2adb8167d9a32/plugin.py#L27-L48

I only added the is_allowed_to_start method, to return helpful error messages to the user... but I guess it would be better if the server return a message to display the message to the user. https://microsoft.github.io/language-server-protocol/specification#window_showMessage