Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backwards compatibility mode silently fails to work if there are e.g. unmet dependencies #799

Closed
36degrees opened this issue Sep 5, 2019 · 1 comment · Fixed by #815
Closed
Labels
🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) 🕔 Hours A well understood issue which we expect to take less than a day to resolve.
Milestone

Comments

@36degrees
Copy link
Contributor

Steps to reproduce:

  1. Create an app/v6/routes.js file that fails to run correctly (for example, by including an dependency that can't be resolved)
  2. Run the kit

Expected result

The kit should surface the error from the routes file.

Actual result

The kit starts, but the v6 routes file is silently ignored.


This is because we catch all errors when trying to include the v6/routes.js file, in case it doesn't exist. We silently drop those errors, and just proceed without enabling compatibility mode:

try {
v6Routes = require('./app/v6/routes.js')
useV6 = true
} catch (e) {
// No routes.js in app/v6 so we can continue with useV6 false
}

I think this may be a fairly likely scenario, because when users update the Prototype Kit they overwrite their package.json file, which means any additional dependencies that were installed are lost. They may then try to use the backwards compatibility mode, but it'll fail silently until they resolve their missing dependencies.

@36degrees 36degrees added 🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) awaiting-triage labels Sep 5, 2019
@joelanman
Copy link
Contributor

joelanman commented Sep 5, 2019

could use fs.existsSync for the check, then require
https://nodejs.org/api/fs.html#fs_fs_existssync_path

@kellylee-gds kellylee-gds added 🕔 Hours A well understood issue which we expect to take less than a day to resolve. Priority: low and removed awaiting-triage labels Sep 11, 2019
36degrees added a commit that referenced this issue Oct 8, 2019
We automatically ‘enable’ compatibility mode if the file app/v6/routes.js exists.

We currently do this by including it within a try/catch block, relying on the empty catch block handling the case where the file does not exist.

However this means that if any exception is encountered whilst requiring that file, we silently fail and do not enable compatibility mode.

This means that if e.g. a user’s v6 routes file includes a dependency which is not installed (which is likely if the user is trying to update their prototype, as doing so nukes any additional dependencies in their package.json file) then this error will not be reported and compatibility mode just won’t work.

Instead, we test for the presence of app/v6/routes.js – if it exists, then we require it. If any exception is encountered in doing so, it’ll be thrown as expected.

Fixes #799
36degrees added a commit that referenced this issue Oct 9, 2019
We automatically ‘enable’ compatibility mode if the file app/v6/routes.js exists.

We currently do this by including it within a try/catch block, relying on the empty catch block handling the case where the file does not exist.

However this means that if any exception is encountered whilst requiring that file, we silently fail and do not enable compatibility mode.

This means that if e.g. a user’s v6 routes file includes a dependency which is not installed (which is likely if the user is trying to update their prototype, as doing so nukes any additional dependencies in their package.json file) then this error will not be reported and compatibility mode just won’t work.

Instead, we test for the presence of app/v6/routes.js – if it exists, then we require it. If any exception is encountered in doing so, it’ll be thrown as expected.

Fixes #799
@36degrees 36degrees added this to the Next milestone Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) 🕔 Hours A well understood issue which we expect to take less than a day to resolve.
Projects
None yet
3 participants