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(compiler-sfc): allow export default substring #7041

Closed
wants to merge 1 commit into from

Conversation

btea
Copy link
Contributor

@btea btea commented Nov 7, 2022

fix #7038

Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

Consider this example:
export const foo = default_

Let's check if the brackets ({}) exist.
export { foo as default }

@sxzz
Copy link
Member

sxzz commented Nov 7, 2022

Questions:

  • (?:expr) is really necessary? It's just used to test here namedDefaultExportRE.test()
  • (?:as)? why as optional? Is that really a valid syntax without as?
    • I think there are only two types of named default export: export { foo as default } and plus from 'xxx'
    • default is a keyword of JavaScript, so it can't be like export { default }. It's an invalid syntax.
    • Answer: export { default } from 'xxx'

FYI I wrote a regexp: /(^|\n|;)\s*export\s*{\s*([\w_$]+)\s*as\s*default/s

UPDATED regex /(^|\n|;)\s*export\s*{.*(\s*[\w_$]+\s+as\s+)?default/

@sxzz sxzz requested a review from antfu November 7, 2022 18:09
@btea
Copy link
Contributor Author

btea commented Nov 9, 2022

For the case without as, maybe can refer #5937

@sxzz
Copy link
Member

sxzz commented Nov 9, 2022

@btea Oh, thanks! I just remembered.

@sxzz
Copy link
Member

sxzz commented Nov 9, 2022

I just updated the regexp, maybe you can try it.

@btea
Copy link
Contributor Author

btea commented Nov 9, 2022

It looks great, but the s flag doesn't seem to be needed.

@btea
Copy link
Contributor Author

btea commented Nov 9, 2022

I tried it but it fails in these cases

expect(
rewriteDefault(
`const a = 1 \n export { a as b, a as default, a as c}`,
'script'
)
).toMatchInlineSnapshot(`
"const a = 1
export { a as b, a as c}
const script = a"
`)
expect(
rewriteDefault(
`const a = 1 \n export { a as b, a as default , a as c}`,
'script'
)

@sxzz
Copy link
Member

sxzz commented Nov 9, 2022

@btea I think it's very hard to check it by regex, I think we could do it by AST. I trying to implement it.

@klyuevtech
Copy link

Hi gays! This new regexp ((^|\n|;)\sexport\s{.(\s[\w_$]+\s+as\s+)?default) could give false positive on a line like this one:
export { foo as bar }; const baz = "default";
Probably we should close the braces?
(^|\n|;)\s*export\s*{.*(\s*[\w_$]+\s+as\s+)?default\s*}

@sxzz
Copy link
Member

sxzz commented Nov 9, 2022

@klyuevtech
Missing the case: export { a, a as default, c }
Matched the case but shouldn't export { somedefault }.

@klyuevtech
Copy link

klyuevtech commented Nov 9, 2022

@sxzz This one seems works well:
(^|\n|;)\s*export((\s*{[\w\s_$,]*([\w_$]+\s+as\s+)+)?|(\s*{\s*)+)default((\s|,)+[\w_$\s]*)?}

@btea
Copy link
Contributor Author

btea commented Nov 9, 2022

@klyuevtech
It doesn't seem to match the following

expect(
rewriteDefault(`export { foo, default } from './index.js'`, 'script')
).toMatchInlineSnapshot(`
"import { default as __VUE_DEFAULT__ } from './index.js'

@sxzz
Copy link
Member

sxzz commented Nov 9, 2022

😮‍💨 I think it's too complex, even if we have a perfect regex.
We have the AST (PR #7068) already, so I think checking it through AST is better than a very complex regex

@znck
Copy link
Member

znck commented Nov 9, 2022

AST based approach handles default in comments and strings, I think works better in all cases. Closing in favour of #7068.

@znck znck closed this Nov 9, 2022
@btea btea deleted the fix/rewriteDefault branch November 9, 2022 23:16
yyx990803 pushed a commit that referenced this pull request Mar 28, 2023
IAmSSH pushed a commit to IAmSSH/core that referenced this pull request May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants