-
Notifications
You must be signed in to change notification settings - Fork 14
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
ensure bundler errors go right up the stack #808
Conversation
🦋 Changeset detectedLatest commit: 6e9ec72 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The preview packages of this pull request have been published. @bigtest/agentInstall using the following command: $ npm install @bigtest/agent@raise-bundler-errors-to-the-top Or update your package.json file: {
"@bigtest/agent": "raise-bundler-errors-to-the-top"
} bigtestInstall using the following command: $ npm install bigtest@raise-bundler-errors-to-the-top Or update your package.json file: {
"bigtest": "raise-bundler-errors-to-the-top"
} @bigtest/interactorInstall using the following command: $ npm install @bigtest/interactor@raise-bundler-errors-to-the-top Or update your package.json file: {
"@bigtest/interactor": "raise-bundler-errors-to-the-top"
} @bigtest/serverInstall using the following command: $ npm install @bigtest/server@raise-bundler-errors-to-the-top Or update your package.json file: {
"@bigtest/server": "raise-bundler-errors-to-the-top"
} @bigtest/suiteInstall using the following command: $ npm install @bigtest/suite@raise-bundler-errors-to-the-top Or update your package.json file: {
"@bigtest/suite": "raise-bundler-errors-to-the-top"
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to kill the CLI if there is a bundler error? or is this just in the event that we can't actually link and load the bundle. It seems to me there are two things:
- The CI command should definitely fail
- the
server
should continue running, but it might make sense to crash if a certain class of errors (what amounts to linkage errors) percolate upwards.
Good point, I think we have different types of exceptions. I think we can say that a test that looks like this: {
test: {
t: TestBuilder {
description: 'No default export',
steps: [],
assertions: [],
children: [],
state: 'step'
},
fileName: 'manifest-54f435368391c9a6f8b73ea545058998181e4f5f0c28e868b684878125ca0ab8.js'
}
} A For unhandled or unexpected exceptions, we crash the CI. |
How do we differentiate the two? |
By the type of exception. As we know, there are no multiple catch blocks in javascript so we could use a type guard or something like: const isUnandledException(err: any) => /// using some tag on our custom Errors and then } catch(error) {
console.debug("[manifest builder] error loading manifest");
bundlerSlice.update(() => ({ type: 'ERRORED', error }));
if (isUnandledException (error)) {
return;
}
throw error; |
closed in favour of #809 |
Fixes #801
This is to ensure bundler errors get raised right to the cli and we give the user a chance to create a github issue:
The original issue was caused by a user with a test file that did not have a default export.
This fix still does not let the user know what the error was.
As I may have mentioned a few times :), I am not a fan of this default export requirement.
But as it stands we are not doing any runtime check.
We could potentially check in the manifest-generator that the files have a default export but doing this on a file by file basis is not great.