Add ESLint and pin next peer dependency range #21

floroz · 2021-05-24T15:08:36Z

This PR introduces linting and upgrades some packages in to next-remote-watch:

Adds next as peer dependencies supporting versions greater than 10.
Static Analysis
- ESLint and Prettier are now applied in the project for better IDE assistance

fix

floroz

I am trying to think what pattern we could use that would avoid the need to use Dependency Injection so deep into the call stack.

This is due to the need to have access to the program and app object.

Any suggestion is welcomed.

package.json

src/server/start.js

src/watcher/on-watch.js

src/server/start.js

\

package.json

floroz · 2021-05-25T15:32:14Z

@jescalan PR is ready, let me know your feedback 😄

jescalan

Hey @floroz! First of all I want to thank you again for all the work you put into this project. It is amazing to see such a drive to contribute to open source, community tooling, and I deeply appreciate it.

That being said, I feel a little uncomfortable about these changes in general, because I am not convinced that they further our goals with this project of making it small, simple, stable, and easy to maintain and contribute to. Originally, the library was comprised of a single file that was maybe 100 LOC and quite easy to read through, update, and understand. After this set of changes, it is now made up of a large network of interconnected files that do exactly the same thing. This represents overall an increase is complexity, and a decrease in understandability and maintainability.

Of course, that tradeoff would be worth it if there was another aspect that was dramatically improved as a result. And I think the primary argument for this is that adding tests makes the library much more stable and reliable, and in theory it needs to be broken into many files to have good test coverage (questionable, but let's assume), and so they are worth the hit we take to clarity and maintainability, and the extra LOC introduced.

However, I am not entirely convinced that the tests added here really add that much stability to the library. For example, one of the things you pointed out that could cause this library to break is that next.js could update its internals and break things. A test to cover whether that has happened or not would be a notable improvement. But I don't think any of the tests added here actually cover this case. Here, we mock the next function. Here, we mock the internal reloader object. And here, we mock the entire reload controller. I may have missed it, and I do apologize if I have, but I think even with these added tests, nextjs internals could change and break this library, and all the tests would still be passing.

I do think there are some situations in which unit tests can be helpful tools, but I'm not sure I see this situation as one of them. What this library is severely lacking is an integration tests -- a test that will tell us if something is actually wrong, or broken. What this PR adds is a lot of extra code and complexity, but as far as I can see, there is no boost to the stability or reliability of the library overall.

I should note that I am just reviewing the code and surely you have a much deeper knowledge of what's going on given that you wrote it. So it's for sure 100% possible that I am missing something here that would really be a large benefit. If so, I would really love if you'd counter-argue my points above, am very open to it, and apologize for misunderstanding.

Aside from the breaking apart of the files and the testing, I think the addition of the linting setup is a definitely great value add, and we should hold on to that regardless. I guess the challenge that I'd put in front of you is whether you'd be able to keep tinkering with this and get it to a point where we have added the most possible value with the least possible complexity. If it were me, I'd probably try to keep the implementation as-is, and see if I could get a bonafide integration test rolling. During initial development, I set up a manual fixture as you could see, where I could test it, but didn't have time to go through the process of figuring out how to fully automate that test.

src/main.js

src/next-dev-server/start.test.js

src/program/create.test.js

Co-authored-by: Jeff Escalante <jescalan@users.noreply.github.com>

floroz · 2021-05-28T06:08:46Z

Hey @floroz! First of all I want to thank you again for all the work you put into this project. It is amazing to see such a drive to contribute to open source, community tooling, and I deeply appreciate it.

That being said, I feel a little uncomfortable about these changes in general, because I am not convinced that they further our goals with this project of making it small, simple, stable, and easy to maintain and contribute to. Originally, the library was comprised of a single file that was maybe 100 LOC and quite easy to read through, update, and understand. After this set of changes, it is now made up of a large network of interconnected files that do exactly the same thing. This represents overall an increase is complexity, and a decrease in understandability and maintainability.

Of course, that tradeoff would be worth it if there was another aspect that was dramatically improved as a result. And I think the primary argument for this is that adding tests makes the library much more stable and reliable, and in theory it needs to be broken into many files to have good test coverage (questionable, but let's assume), and so they are worth the hit we take to clarity and maintainability, and the extra LOC introduced.

However, I am not entirely convinced that the tests added here really add that much stability to the library. For example, one of the things you pointed out that could cause this library to break is that next.js could update its internals and break things. A test to cover whether that has happened or not would be a notable improvement. But I don't think any of the tests added here actually cover this case. Here, we mock the next function. Here, we mock the internal reloader object. And here, we mock the entire reload controller. I may have missed it, and I do apologize if I have, but I think even with these added tests, nextjs internals could change and break this library, and all the tests would still be passing.

I do think there are some situations in which unit tests can be helpful tools, but I'm not sure I see this situation as one of them. What this library is severely lacking is an integration tests -- a test that will tell us if something is actually wrong, or broken. What this PR adds is a lot of extra code and complexity, but as far as I can see, there is no boost to the stability or reliability of the library overall.

I should note that I am just reviewing the code and surely you have a much deeper knowledge of what's going on given that you wrote it. So it's for sure 100% possible that I am missing something here that would really be a large benefit. If so, I would really love if you'd counter-argue my points above, am very open to it, and apologize for misunderstanding.

Aside from the breaking apart of the files and the testing, I think the addition of the linting setup is a definitely great value add, and we should hold on to that regardless. I guess the challenge that I'd put in front of you is whether you'd be able to keep tinkering with this and get it to a point where we have added the most possible value with the least possible complexity. If it were me, I'd probably try to keep the implementation as-is, and see if I could get a bonafide integration test rolling. During initial development, I set up a manual fixture as you could see, where I could test it, but didn't have time to go through the process of figuring out how to fully automate that test.

Hi Jeff,

Thanks for your reply and the thorough review.

Contributing to others' projects is always a great learning experience for me, and an opportunity to work outside the standards and conventions of my day-to-day work.

I understand your concern about readability and LOC.

Having everything into a single file might help the reader glance back and forth between lines, but ultimately is a design choice that brings a great trade-off as we have procedural code without any encapsulation, where it's hard to distinguish at first what is responsible for what.

If we separate the responsibilities within the binary, we can distinguish some separate entities:

an instance of the next dev server
a commander CLI
a watcher configuration and callback
a custom express server with an endpoint for hot reloading

Keeping all those things within the same file might help reading or to make a quick change, but will ultimately impair the capacity to confidently extend the functionalities and have confidence that change to a line will not have an impact 10 lines below.

The only way to overcome this problem is to separate the code into modules representing the responsibility that each unit of code represents.

The ability to maintain a degree of simplicity and readability during this process the developer's responsibility and I think things could be further improved here, and I would welcome any suggestion you might have on how to better structure this project.

Regarding the unit test, I think some of them bring value, as explained with the example of the commander test, or making sure our function creating the express server does what we expect it to do.

However, I do agree that we would need an integration test that would bring the confidence that the library is working. I have a few ideas about that.

What I would recommend, is perhaps having a synchronous chat over Slack/Zoom in the next week, to better coordinate our intent and have a shared understanding of how this project should look like, and map the way forward.

Let me know :)

jescalan · 2021-05-30T20:13:53Z

Sounds good - let's find a time to meet. Could you reach out via email so we can set it up? You can find my email here 😁

floroz · 2021-06-03T14:48:06Z

Sounds good - let's find a time to meet. Could you reach out via email so we can set it up? You can find my email here 😁

Awesome, I've sent you an email 😄

floroz · 2021-06-07T17:07:04Z

.eslintrc

+      "files": ["bin/*"],
+      "extends": [
+        "eslint:recommended",
+        "plugin:node/recommended",


we create an override here to avoid applying the node settings and plugin to the rest of the project (which may include browser environments and next.js files)

floroz · 2021-06-07T17:07:56Z

bin/next-remote-watch

@@ -76,7 +78,7 @@ app.prepare().then(() => {

  // special handling for mdx reload route
  const reloadRoute = express.Router()
-  reloadRoute.use(bodyParser.json())
+  reloadRoute.use(express.json())


bodyparser is now baked in express

floroz · 2021-06-07T17:08:41Z

package.json

    "chalk": "^4.0.0",
    "chokidar": "^3.4.0",
    "commander": "^5.0.0",
    "express": "^4.17.1"
  },
+  "peerDependencies": {
+    "next": ">=10"


@jescalan would this count as range ? As per our discussion, this should include all versions major than next 10?

yeah this is perfect!

floroz · 2021-06-07T17:12:18Z

hey @jescalan, I have reduced the scope of this PR to only introduce eslint, prettier and update the package.json with the peer dependency range.

Will introduce integration testing in the next PR 😄

jescalan

This looks great! 🎉

jescalan · 2021-06-07T17:21:02Z

package.json

    "chalk": "^4.0.0",
    "chokidar": "^3.4.0",
    "commander": "^5.0.0",
    "express": "^4.17.1"
  },
+  "peerDependencies": {
+    "next": ">=10"


yeah this is perfect!

BRKalow

🚀

jescalan · 2021-06-07T19:11:22Z

First contribution to this library congrats! 🎉🎉🎉🎉

Daniele Tortora added 14 commits May 21, 2021 12:15

chore: 🤖 install jest eslint and draft testing strategy plan

1b3293e

✅ Closes: hashicorp#19

chore: 🤖 add eslint npm script

3d976f0

test: 💍 add unit tests and refactor file structure

c036c3d

chore: 🤖 rename import

f392c1d

test: 💍 add test for start watcher

1fef475

test: 💍 add test for watcher

ac60ebe

chore: 🤖 add nextjs as peer dependency

d1d3423

refactor: 💡 better code encapsulation and naming

e3bf8c0

refactor: 💡 better naming for files

08a30fa

refactor: 💡 more file name and import change for better clarity

987372e

chore: 🤖 removed eslint comment

5825452

chore: 🤖 remove from git testing plan

4a4e411

chore: 🤖 fix import

5dd97fa

chore: 🤖 eslint plugin import

673a408

fix

floroz force-pushed the daniele-tortora/unit-test-main branch from 8ebc5a4 to 673a408 Compare May 24, 2021 15:23

floroz commented May 24, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/server/start.js Outdated Show resolved Hide resolved

src/watcher/on-watch.js Outdated Show resolved Hide resolved

floroz commented May 24, 2021

View reviewed changes

src/server/start.js Outdated Show resolved Hide resolved

test: 💍 test coverage and refactor for express server

40fcdba

\

floroz marked this pull request as ready for review May 25, 2021 10:42

chore: 🤖 eslint group imports

f80b875

floroz commented May 25, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Daniele Tortora added 7 commits May 25, 2021 12:55

refactor: 💡 change import style for better clarity

9e59b22

chore: 🤖 function renaming

171253f

refactor: 💡 create router within scope of express start functio

065bac7

refactor: 💡 migrate to arrow function

370d0e5

chore: 🤖 eslint import fix

812b251

docs: ✏️ add comment for deprecated api

70f296a

chore: 🤖 semver support for next 10

27f1252

refactor: 💡 change test to use jest utils

c19829f

jescalan reviewed May 26, 2021

View reviewed changes

src/main.js Outdated Show resolved Hide resolved

src/next-dev-server/start.test.js Outdated Show resolved Hide resolved

src/program/create.test.js Outdated Show resolved Hide resolved

Update src/main.js

dfa3087

Co-authored-by: Jeff Escalante <jescalan@users.noreply.github.com>

feat: restore project structure and expand linting config

fcfeb1f

floroz commented Jun 7, 2021

View reviewed changes

floroz changed the title ~~Project Upgrade: ESLint, test coverage, Files refactor~~ Add ESLint and pin next peer dependency range Jun 7, 2021

floroz requested a review from jescalan June 7, 2021 17:12

jescalan approved these changes Jun 7, 2021

View reviewed changes

BRKalow approved these changes Jun 7, 2021

View reviewed changes

jescalan merged commit 2d1a5b6 into hashicorp:master Jun 7, 2021

floroz deleted the daniele-tortora/unit-test-main branch June 8, 2021 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ESLint and pin next peer dependency range #21

Add ESLint and pin next peer dependency range #21

floroz commented May 24, 2021 •

edited

Loading

floroz left a comment

floroz commented May 25, 2021

jescalan left a comment

floroz commented May 28, 2021 •

edited

Loading

jescalan commented May 30, 2021

floroz commented Jun 3, 2021

floroz Jun 7, 2021

floroz Jun 7, 2021

floroz Jun 7, 2021

jescalan Jun 7, 2021

floroz commented Jun 7, 2021

jescalan left a comment

jescalan Jun 7, 2021

BRKalow left a comment

jescalan commented Jun 7, 2021

Add ESLint and pin next peer dependency range #21

Add ESLint and pin next peer dependency range #21

Conversation

floroz commented May 24, 2021 • edited Loading

floroz left a comment

Choose a reason for hiding this comment

floroz commented May 25, 2021

jescalan left a comment

Choose a reason for hiding this comment

floroz commented May 28, 2021 • edited Loading

jescalan commented May 30, 2021

floroz commented Jun 3, 2021

floroz Jun 7, 2021

Choose a reason for hiding this comment

floroz Jun 7, 2021

Choose a reason for hiding this comment

floroz Jun 7, 2021

Choose a reason for hiding this comment

jescalan Jun 7, 2021

Choose a reason for hiding this comment

floroz commented Jun 7, 2021

jescalan left a comment

Choose a reason for hiding this comment

jescalan Jun 7, 2021

Choose a reason for hiding this comment

BRKalow left a comment

Choose a reason for hiding this comment

jescalan commented Jun 7, 2021

floroz commented May 24, 2021 •

edited

Loading

floroz commented May 28, 2021 •

edited

Loading