Refactoring: better boundaries for different parts of the project by duailibe · Pull Request #4364 · prettier/prettier

duailibe · 2018-04-24T13:09:48Z

Analyzing our current build of the bin-prettier.js we distribute on NPM and after several tries of having a better build for web I've noticed our current graph of dependencies is quite a mess.

I've started a bit of refactoring here, gonna try summarize my rationale and what changed here:

we need to have core functionality independent of infrastructure. My rationale is: we can build another "entry" for prettier that knows how to load plugins (like in a web context, via importScripts) that would simply pass the loaded plugins to the core parts. So I've removed loadPlugins calls from main folder
our bin-prettier.js is basically index.js + stuff for CLI, but it also requires index.js meaning users end up loading a lot of code twice. ideally the CLI would always use the public API as well (in index.js) so I've removed some direct calls to getSupportInfo to go through prettier.getSupportInfo.

I need help from people (cc @ikatyang and @azz) to understand a few things we have I found a bit complicated:

what is the difference between main/options.js's normalize function and the normalizers in main/options-normalizers.js. Do we need 3 normalizers? How can we make this simpler? We could rename them too, because we have way too much normalizers in the code base 😂
I've noticed we have duplicated logic to log unknown options and suggest another option with leven in two different places (one for API and one for CLI) but one ends up also including the other in the final build. Is there a way to reuse them? Should make sense to extract the core.

This is still WIP, you guys think this is worth pursuing? I can also share my views for the web build if all of this don't make sense.

ikatyang · 2018-04-24T13:46:43Z

what is the difference between main/options.js's normalize function and the normalizers in options-normalizer. How can we make them simpler?

options.js is a prettier-specific normalizer which uses options-*.js.
options-*.js could be a standalone package, I originally thought it'd be better there for maintainability reason, but now it seems extracting it would be better(?), what do you think?

I've noticed we have duplicated logic to log unknown options and suggest another option with leven in two different places (one for API and one for CLI) but one ends up also including the other in the final build. Is there a way to reuse them? Should make sense to extract the core.

There're actually 3 places (api options, cli options, and cli --help) where the first two are from options-normalizer, and the core is just a few lines:

prettier/src/main/options-normalizer.js

Lines 92 to 97 in 50101e9

    
           const suggestedOptionInfo = optionInfos.find( 
        
             optionInfo => leven(optionInfo.name, key) < 3 
        
           ); 
        
           if (suggestedOptionInfo) { 
        
             messages.push(`Did you mean ${JSON.stringify(suggestedOptionInfo.name)}?`); 
        
           }

This is still WIP, you guys think this is worth pursuing?

I guess this is what you mean:

core (no fs, web compatible, including format(), formatWithCursor())
api (require core and fs)
cli (require api and fs)
plugins (independent, web compatible)

👍

duailibe · 2018-04-24T13:53:56Z

@ikatyang yeah I think you explained it better than me 😂

options.js is a prettier-specific normalizer which uses options-*.js.

options-*.js could be a standalone package, I originally thought it'd be better there for maintainability reason, but now it seems extracting it would be better(?), what do you think?

Thank you for this! It helped me understand it better, but I'm still confused about what is options-normalizer.js responsibility.

ikatyang · 2018-04-24T13:56:57Z

validate options, redirect deprecated options and apply default value.

ikatyang · 2018-04-24T13:58:24Z

And yes, I'm not good at naming, so I just named it normalizer. 😂

j-f1 · 2018-04-24T14:39:21Z

 const printDocToDebug = doc.debug.printDocToDebug;

+function withPlugins(opts) {
+  opts = Object.assign({}, opts);


This line is unnecessary.

It was not but I removed it anyway :)

…/resolver.js

j-f1 · 2018-04-24T16:41:35Z

        key === "parent" ||
-        key === "prev"
+        key === "prev" ||
+        key === "__location"


What do you think about replacing this with ["loc", "range", "raw", "comments", "parent", "prev", "__location"].indexOf(key) !== -1?

duailibe · 2018-04-24T17:35:59Z

@ikatyang I think there's still some complexity that could be simplified but I think this is already good enough to have a web entry point which is what I want to achieve =)

ikatyang · 2018-04-25T12:00:09Z

  doc,

  resolveConfig: config.resolveConfig,
+  resolveConfigFile: config.resolveConfigFile,


~~We should also update api.md.~~ (bf68797)

ikatyang · 2018-04-25T12:08:58Z

-    "<rootDir>/src/deprecated.js"
+    "<rootDir>/src/doc/doc-debug.js",
+    "<rootDir>/src/main/massage-ast.js",
+    "<rootDir>/src/main/deprecated.js"


deprecated.js is removed.

ikatyang · 2018-04-25T12:18:32Z

 const jsOptions = require("../language-js/options");

-// format based on https://github.com/prettier/prettier/blob/master/src/common/support.js
+// format based on https://github.com/prettier/prettier/blob/master/src/core/options.js


core/options.js -> core/core-options.js

(I'm not sure why there's only one file in core/ but I guess that's just your first step.)

I was hoping to find a better place for it actually.. gonna move it to main/

ikatyang · 2018-04-25T12:20:30Z

 const jsOptions = require("../language-js/options");

-// format based on https://github.com/prettier/prettier/blob/master/src/common/support.js
+// format based on https://github.com/prettier/prettier/blob/master/src/core/options.js


core-options.js

ikatyang · 2018-04-25T12:24:25Z

 const CATEGORY_JAVASCRIPT = "JavaScript";

-// format based on https://github.com/prettier/prettier/blob/master/src/common/support.js
+// format based on https://github.com/prettier/prettier/blob/master/src/core/options.js


core-options.js

ikatyang · 2018-04-25T12:25:53Z

 const CATEGORY_MARKDOWN = "Markdown";

-// format based on https://github.com/prettier/prettier/blob/master/src/common/support.js
+// format based on https://github.com/prettier/prettier/blob/master/src/core/options.js


core-options.js

duailibe · 2018-04-25T16:29:47Z

Let's hope for the best 🙏 😂

lydell · 2018-04-25T17:09:52Z

Thanks @duailibe ! 🎉

duailibe added 10 commits April 23, 2018 20:26

stop automatically loading plugins in getSupportInfo

9a4765a

move arrayify to separate file

23e9809

rename

6725474

remove unnecessary call to loadPlugins

f36910f

remove loadPlugins call from main/options.js normalize

3b6f381

embed get-plugin.js

9e4c97c

rename

6bc3756

move file

0237987

Merge branch 'master' of github.com:prettier/prettier into refactor

4ab0dce

fix tests

2902537

duailibe added 3 commits April 24, 2018 11:05

Merge branch 'master' of github.com:prettier/prettier into refactor

d7e23c2

remove unused function

9dea29e

remove unused file

9e5e38a

j-f1 reviewed Apr 24, 2018

View reviewed changes

duailibe mentioned this pull request Apr 24, 2018

fix(cli): do not early exit for --debug-check #2987

Merged

duailibe added 2 commits April 24, 2018 12:33

fix infinite recursion

49f6187

refactor

fd7a315

duailibe added the status:wip Pull requests that are a work in progress and should not be merged label Apr 24, 2018

duailibe added 2 commits April 24, 2018 12:48

expose resolveConfigFile in API and remove CLI dependency on config…

a11a052

…/resolver.js

now fix infinite recursion bug

f25200d

j-f1 reviewed Apr 24, 2018

View reviewed changes

duailibe added 2 commits April 24, 2018 14:49

refactor

ad26546

fix node 4

4e908cd

duailibe removed the status:wip Pull requests that are a work in progress and should not be merged label Apr 24, 2018

duailibe changed the title ~~[RFC] Refactoring: better boundaries for different parts of the project~~ Refactoring: better boundaries for different parts of the project Apr 24, 2018

ikatyang reviewed Apr 25, 2018

View reviewed changes

duailibe added 3 commits April 25, 2018 12:28

Merge branch 'master' of github.com:prettier/prettier into refactor

24dfacc

update docs

bf68797

review changes

e8f9d69

ikatyang approved these changes Apr 25, 2018

View reviewed changes

duailibe merged commit 67f1c48 into prettier:master Apr 25, 2018

duailibe deleted the refactor branch April 25, 2018 16:29

This was referenced Apr 26, 2018

Expose config.resolveConfigFile in API? #3451

Closed

add resolveConfigWithFilePath API #3846

Closed

Separate entry for web bundle #4386

Merged

Revamp Build System #3574

Closed

ikatyang mentioned this pull request May 4, 2018

Fix plugin API in globally installed Prettier and introduce optional --plugin-search-dir #4192

Merged

lipis added this to the 1.13 milestone May 9, 2018

lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 7, 2018

lock Bot locked as resolved and limited conversation to collaborators Aug 7, 2018

Uh oh!

Conversation

duailibe commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikatyang commented Apr 24, 2018

Uh oh!

duailibe commented Apr 24, 2018

Uh oh!

ikatyang commented Apr 24, 2018

Uh oh!

ikatyang commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duailibe commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikatyang Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duailibe commented Apr 25, 2018

Uh oh!

lydell commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

duailibe commented Apr 24, 2018 •

edited

Loading

duailibe commented Apr 24, 2018 •

edited

Loading

ikatyang Apr 25, 2018 •

edited

Loading