jscheid / prettier.el

Prettier code formatting for Emacs.
GNU General Public License v3.0
167 stars 12 forks source link

Add an option to set prettier module path #1

Closed asbish closed 4 years ago

asbish commented 4 years ago

Yarn switched thier package management to Plug'n'Play style in Yarn berry and this change breaks our searching method for Prettier because there are no longer node_modules directories.

One possible fix for this, I'd like to add a prettier-module-path option to use Editor SDKs for Prettier they have provided as described below.

(setq-local prettier-module-path (expand-file-name "~/.vscode/pnpify/prettier"))
jscheid commented 4 years ago

Hi, thank you for your contribution.

I'm hesitant to merge as-is mainly because I'd like to avoid asking users to hard-wire paths. The current way of locating prettier was designed so that when you switch between different projects, where each project might use a different version of prettier, it will always work correctly. This could theoretically be achieved with something like your approach by using dir-locals.el for example, but I'm hoping that we can find a solution that requires less maintenance work.

One possibility that I would like to explore is the following: check package.json to see if the pnp flag is set (or perhaps check for existance of .pnp.js) and if found, use yarn node to run our JS code instead of just node.

There also appears to be an API that might allow us to locate the package.

I'm going to try and find the time soon for digging deeper into this. In the meantime, if you wanted to have a look at these alternate approaches yourself that would be most welcome.

Thanks again!

asbish commented 4 years ago

Thank you for your response and kind support. After reading your indication, I've realized that less maintenance and zero-configuration are much preferable. I still remember when I used this package for the first time I was able to configure without hassle :blush:

I tried the way as you mentioned. It works nicely on Node10/Yarn1.17.3(classic) and Node12/Yarn1.22.1(berry@2.0.0-rc.29) environments. In my understanding Yarn P'n'P will not work if .pnp.js doesn't exist. Therefore I don't think it's nessesary to check for pnp flag in package.json.

If you agree I'll open new PR after I check more environment cases and dive in Yarn berry in a week.

(defun prettier--create-process (server-id)
  ...
  (start-file-process "prettier" buf "yarn" "node" "--eval" ...)
  ...
)
function getPrettierForPath(filepath) {
  if (process["versions"]["pnp"]) {
    const module = externalRequire("module");
    const createRequire = module["createRequire"] || module["createRequireFromPath"]; // v12.2.0 || v10.12.0
    const targetRequire = createRequire(filepath); // Search .pnp.js
    const resolved = targetRequire.resolve('prettier');
    const prettier = targetRequire(resolved);
    prettierCache.set(filepath, prettier);
    return prettier;
  } else {
    ...
  }
}
jscheid commented 4 years ago

Thank you, I agree this looks like a preferable approach, please go ahead. Could you also make sure that it keeps working with npm and (ideally) pnpm?

asbish commented 4 years ago

Thank you for the message. I'll make it sure that interoperability of npm works. I'll use yarn node when the project has .pnp.js.

jscheid commented 4 years ago

Am I correct in assuming that this can be closed now that #3 is done?

asbish commented 4 years ago

Yes. Of course. Thank you for merging!