Skip to content

Plugins: If minVersion >= 3.0.0, don't "pre-run" .setOptions()#3247

Merged
matthew-dean merged 4 commits intoless:masterfrom
calvinjuarez:plugins-for-v3-run-setoptions-once
Jul 3, 2018
Merged

Plugins: If minVersion >= 3.0.0, don't "pre-run" .setOptions()#3247
matthew-dean merged 4 commits intoless:masterfrom
calvinjuarez:plugins-for-v3-run-setoptions-once

Conversation

@calvinjuarez
Copy link
Copy Markdown
Member

This is just what I was mentioning in #3233. Turned out to be simple enough. Tests were updated and a 3.x test was added. If no minVersion is specified by a plugin, setOptions() is run twice to be safe. Maybe no specification should "assume v3+"? That could break more old plugins created without the minVersion property.

Copy link
Copy Markdown
Member

@matthew-dean matthew-dean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calvinjuarez I really like this. Much cleaner. Maybe remove .editorconfig? (I realize I made the opposite argument for the .vscode file, but that file was for launching tests, whereas this is config?)

Also, CI tests are failing?

@calvinjuarez
Copy link
Copy Markdown
Member Author

.editorconfig is meant to be a way to share and enforce coding standards across editors. I can keep it out, but I think it has value in the repository. (See https://editorconfig.org). I don’t mind ignoring it, if it’s not seen as generally useful, but it’s a nice way for me to contribute using my preferred IDE and others to use theirs without having to manually change editor settings (I use tab indentation on my work and personal stuff, for example).

As for the CI failures, I wondered about that. I’m not hip to CI, so I could use some help knowing how to review what’s broken so I can fix it.

@calvinjuarez
Copy link
Copy Markdown
Member Author

Okay, I think I see what the tests are verifying. I’ll try to test on all version soon of Node in the tests. Stay tuned.

@matthew-dean
Copy link
Copy Markdown
Member

matthew-dean commented Jul 1, 2018

@calvinjuarez The error is the same as on main. Which is: SyntaxError: setOptions() not called before install.

It seems like there's some race condition around setting options and install? Somewhere there must be an async callback. So you didn't cause this, but since you're looking at set options, do you think you could track it down? It doesn't happen every time, and console logging causes it to stop happening, I think? (Because it's slowing down the race.)

DERP. I see my mistake. Fixing.

@matthew-dean
Copy link
Copy Markdown
Member

Please merge again from master and it should work. [crosses fingers]

@matthew-dean matthew-dean merged commit d542512 into less:master Jul 3, 2018
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.

2 participants