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

fix(ssrTransform): continuous exports parsing error #6407

Merged
merged 1 commit into from
Feb 26, 2022

Conversation

ygj6
Copy link
Member

@ygj6 ygj6 commented Jan 6, 2022

Description

fix: #6395

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ygj6 ygj6 marked this pull request as ready for review January 6, 2022 12:22
@MatisLepik
Copy link

I think there are other kinds of exports that are still failing with this solution. Try adding this to the end of continuous-exports/index.js (without a linebreak)

export { FunctionFromOtherFile } from './otherFile'; 

Result will be:

TypeError: __vite_ssr_import_10__.SecondClass is not a constructor

@ygj6
Copy link
Member Author

ygj6 commented Jan 8, 2022

Not an elegant solution, but it does the trick, it is better to find the root cause and fix it, see my #6395 (comment)

@neb
Copy link

neb commented Jan 31, 2022

As described in my verbose #6395 (comment) I think the (mis)use of s.appendRight() is indeed the root cause here and there's no upstream bug nor a broad structural bug.

The node.endnode.start replacements shouldn't be necessary and are resulting in odd ordering. (e.g. It produces the export of class B before the class itself in that last test.)

I believe the result is cleanest with only the s.appendRight()s.appendLeft() fix and the transplant of its newline to the end of the string.

(I'd be happy to provide a patch or a separate PR if that would be the best way.)

@ygj6
Copy link
Member Author

ygj6 commented Jan 31, 2022

@neb Good catch! But what do you mean about this:

I believe the result is cleanest with only the s.appendRight() → s.appendLeft() fix and the transplant of its newline to the end of the string.

I feel like I've already done this (s.appendRight() → s.appendLeft()) in the commit 6413d2c, please correct me if I'm missing anything :)

@neb
Copy link

neb commented Feb 22, 2022

@ygj6 Sorry I wasn't clearer about that part! My point was the changes of node.end to node.start in ssrTransform.ts should be omitted, I think. AFAICT they don't improve anything and in fact they result in odd ordering. (Note the exports of classes before class definitions in some of the updated snapshots.)

@ygj6
Copy link
Member Author

ygj6 commented Feb 23, 2022

@neb Sorry I misunderstood you earlier, now that I understand, I pushed the latest code, let's see if that's correct.

Comment on lines 71 to 72
`\nObject.defineProperty(${ssrModuleExportsKey}, "${name}", ` +
`{ enumerable: true, configurable: true, get(){ return ${local} }});`
Copy link

@neb neb Feb 25, 2022

Choose a reason for hiding this comment

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

@ygj6 This version works for me, thanks!

The only other very small thing I'd suggest is to move the \n from the beginning to the end of the appended string or just add another one at the end. This obviously wouldn't affect execution and is thus not necessary, but }});function fn2() { in the snapshot would be more legible with a newline after the ;. Just a thought.

Either way, glad to get to the bottom of this bug!

Edit: Oops I didn't mean to leave this comment as a review. I don't count as a real reviewer of course.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @neb, thanks for your review and input on this PR! If you think that this can be further improved, please create a new PR, and let's discuss the addition of the new line out of the context of this bug.

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 26, 2022
@patak-dev patak-dev merged commit 3012541 into vitejs:main Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doesn't export function in SSR if there is other exported function right after
6 participants