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

[feat] Add errorMode option to compile to allow continuing on errors (and mark them as warnings) #6194

Merged
merged 12 commits into from
Jul 17, 2021

Conversation

SomaticIT
Copy link
Contributor

@SomaticIT SomaticIT commented Apr 13, 2021

Description

This PR adds a new option errorMode to CompileOptions to allow continuing the compilation process when errors occured.
When set to warn, this new option will indicate to Svelte that it should log errors as warnings and continue compilation.

This allows (notably) preprocessors to compile the markup to detect vars in markup before preprocessing (in this case: script and style tags are stripped so it can produce a lot of errors).

To avoid breaking change, the old behavior is still the default option (throw).

This PR is part of a work on the svelte-preprocess side to improve the preprocessing of TypeScript files:
sveltejs/svelte-preprocess#318

Some features were implemented in the following PRs:

Details

This PR does the following:

  1. It allows the compiler to pass error as warnings instead of throwing
  2. It enforces return after all component.error() calls to avoid breaking the compilation
  3. It includes a test case for this option
  4. It includes a documentation for this option

Review

During the implementation, I detected an unreachable code.
To allow the review, I only commented this line: https://github.com/sveltejs/svelte/pull/6194/files#r612613005

I think that it could be safely removed for 2 reasons:

  1. It is unreachable (errors stop the process)
  2. It does a Set.add in a Set.has

What do you think?

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

Thanks

- allow compiler to pass error as warnings
- enforce stops after errors during compilation
- should review Element.ts:302
- add a test case for errorMode
- adding documentation
code: 'duplicate-slot-attribute',
message: `Duplicate '${name}' slot`
});

component.slot_outlets.add(name);
// this code was unreachable. Still needed?
// component.slot_outlets.add(name);
Copy link
Contributor Author

@SomaticIT SomaticIT Apr 13, 2021

Choose a reason for hiding this comment

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

As indicated, this code was unreachable before this PR.

I think we could safely remove it since:

  1. It is unreachable 😁
  2. It does set.add in a set.has (what ?)

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

In most cases you do return component.error(.., I'd keep it consistent and do that in the other places, too.

@SomaticIT
Copy link
Contributor Author

I couldn't because TypeScript was not happy when I do this in a constructor.

If you look, all return component.error(... are outside of the constructor scope and separated return; are used in the constructor scope.

@benmccann benmccann changed the title Adding errorMode option to compile to allow continuing on errors (and mark them as warnings) [feat] Add errorMode option to compile to allow continuing on errors (and mark them as warnings) Jun 30, 2021
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 The same mode could be of use later on in a different PR when being more forgiving while parsing (not compiling, which is this PR), helping language tools.

I'm not sure what the unreachable code was trying to do, that might be something for @Conduitry / @tanhauhau to look at, maybe you accidentally discovered a bug.

@dummdidumm dummdidumm merged commit 0cf5511 into sveltejs:master Jul 17, 2021
@SomaticIT SomaticIT deleted the @feature/compile-error-mode branch July 20, 2021 08:55
@SomaticIT
Copy link
Contributor Author

Thank you for merging this PR, I will restart my work on the svelte-preprocess. I have an idea: maybe we could enable the "double" compilation only when svelte-preprocess is running in dev mode.

@Conduitry
Copy link
Member

This has been released in 3.39.0.

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.

3 participants