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 multiline re-export of the default export #549

Merged
merged 2 commits into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@kylekthompson
Contributor

kylekthompson commented Jan 31, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Fixes the highlighting when re-exporting a default export:

export {
  default as alias,
} from 'module';

screen shot 2018-01-31 at 12 57 07 pm

Alternate Designs

Find an alternate regex.

Benefits

The export will be properly tokenized.

Possible Drawbacks

There could be a performance hit with the new regex.

Applicable Issues

I actually did find one! #271

@kylekthompson kylekthompson changed the title from **WIP** Fix multiline re-export of the default export to Fix multiline re-export of the default export Mar 3, 2018

@kylekthompson

This comment has been minimized.

Contributor

kylekthompson commented Mar 3, 2018

@50Wliu sorry it's taken me so long to get back to this!

So, I've tracked down the issue with this; the problem is that the multiline re-export is falling into the from-less matches here.

My proposed change is to search for from across multiple lines, though I'm running into some issues with xregexp. Are we not able to use the s flag?

@MaximSokolov

This comment has been minimized.

Member

MaximSokolov commented Mar 3, 2018

My proposed change is to search for from across multiple lines

This is not possible due to limitation of the regex engine. It can be fixed by removing this pattern and changing invalid.illegal -> variable.language.default here, so default always would be legal inside export statement

@kylekthompson

This comment has been minimized.

Contributor

kylekthompson commented Mar 3, 2018

@MaximSokolov I had considered doing that, but it breaks this case:

export { default as alias };

Maybe it's okay that we don't highlight this as invalid?

@MaximSokolov

This comment has been minimized.

Member

MaximSokolov commented Mar 3, 2018

Yeah, it's fine. Linting is not a priority for this package

@kylekthompson

This comment has been minimized.

Contributor

kylekthompson commented Mar 3, 2018

updated the fix. Thanks for the quick feedback on this!

@50Wliu 50Wliu merged commit e4bc190 into atom:master Mar 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kylekthompson kylekthompson deleted the kylekthompson:fix-multiline-default-reexport branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment