Skip to content

Feature/require over import - #4

Merged
sblaurock merged 4 commits into
masterfrom
feature/require-over-import
Oct 3, 2019
Merged

Feature/require over import#4
sblaurock merged 4 commits into
masterfrom
feature/require-over-import

Conversation

@sblaurock

@sblaurock sblaurock commented Oct 3, 2019

Copy link
Copy Markdown
Contributor
  • import and export are only natively supported in Node 12. Revert to using require to increase compatibility with older versions of Node.

All tests pass. Code coverage is stable.

@sblaurock
sblaurock requested review from jjl014 and rubikzube October 3, 2019 19:28
Comment thread src/modules/search.js
@@ -1,7 +1,7 @@
/* eslint-disable import/prefer-default-export, object-curly-newline */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can remove the disabling of import/prefer-default-export here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

Comment thread src/modules/autocomplete.js Outdated
}

throw new Error(response.statusText);
throw response;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we throwing the entire response here? it's not an error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is - if not, response.ok would be true.

Comment thread src/modules/recommendations.js Outdated
}

throw new Error(response.statusText);
throw response;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we throwing the entire response here?

Comment thread src/modules/search.js Outdated
}

throw new Error(response.statusText);
throw response;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we throwing the entire response here?

Comment thread src/modules/search.js Outdated
}

throw new Error(response.statusText);
throw response;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we throwing the entire response here?

@sblaurock
sblaurock merged commit fb6b04d into master Oct 3, 2019
@sblaurock
sblaurock deleted the feature/require-over-import branch October 3, 2019 21:16
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