Skip to content

Revert "chore: CompileDiagnostic no longer extends Error" #13937

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Revert "chore: CompileDiagnostic no longer extends Error" #13937

wants to merge 1 commit into from

Conversation

paoloricciuti
Copy link
Member

Reverts #13651

Closes #13933 (at least aprtially).

The PR was to allow the Error to be passed via postMessage from the Worker in the playground...but this caused the issue in #13933 because vite-plugin-svelte throw the error after converting it to a RollupError and it's showing the stack when the build fails.

We can find another way to make this work cross Worker but i think we should revert this PR in the meantime.

Output before:

image

Output after:

image

Another option could be to generate a stack and add a stack property on the element like this

/** @implements {ICompileDiagnostic} */
export class CompileDiagnostic {
    name = 'CompileDiagnostic';
    stack = undefined;

    /**
     * @param {string} code
     * @param {string} message
     * @param {[number, number] | undefined} position
     */
    constructor(code, message, position) {
        this.code = code;
        this.message = message;
        this.stack = new Error().stack;

but this feels a bit weird (it would solve the error in the playground (because it doesn't extend Error anymore) while keeping a good looking error if we fail on build (although i wonder if the reason postmessage was failing was because of the stack property that maybe adds too much stuff...i don't know).

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2024

⚠️ No Changeset found

Latest commit: 76ca766

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Oct 25, 2024 

pnpm add https://pkg.pr.new/svelte@13937

commit: 76ca766

@trueadm
Copy link
Contributor

trueadm commented Oct 25, 2024

Can't this be solved in vite-plugin-svelte instead?

@benmccann
Copy link
Member

Hmm, that's tricky. I think if we solved it in vite-plugin-svelte then we'd have to deprecate vite-plugin-svelte 4.0.0 because it's basically unusable at the moment and we'd need everyone on it to upgrade. CC @dominikg

The same might be true for rollup-plugin-svelte and svelte-loader. Someone would need to check those

@paoloricciuti
Copy link
Member Author

Can't this be solved in vite-plugin-svelte instead?

The stack can't be generated while in the plugin and if we don't extend Error or add the stack from an error it will not have the right positions. I think we should do this and fix the worker thing (which btw i'm pretty sure it happened before and i fixed it in some PR...let me see if i can find it).

@paoloricciuti
Copy link
Member Author

This is how i fixed it before: #12394 imho we should do the same now

@trueadm
Copy link
Contributor

trueadm commented Oct 25, 2024

What I mean is that the stack is useless here. We don't need it, so can we provide an empty stack instead to make this work?

@paoloricciuti
Copy link
Member Author

The stack might not be useful but without the stack it doesn't even print the frame and the actual error (and i think it's actually vite that is displaying this stuff). Considering the playground is fixable and the Diagnostic is an Error indeed i would fix this here and fix the playground too (i'm actually about to open a simiilar PR on svelte.dev

@trueadm
Copy link
Contributor

trueadm commented Oct 25, 2024

I mean having an empty stack might convince Vite that it's an error and thus print the error message, even though it's not an error?

@paoloricciuti
Copy link
Member Author

I mean having an empty stack might convince Vite that it's an error and thus print the error message, even though it's not an error?

It does...ok let me close this and open a new one to add a stack property on CompileDiagnostic!

@paoloricciuti paoloricciuti mentioned this pull request Oct 25, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Space before @html breaks the build
3 participants
@paoloricciuti @trueadm @benmccann