Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_formatter): import sorting #2512

Closed
wants to merge 1 commit into from

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Apr 28, 2022

Summary

This PR adds a experimental logic to the formatter to sort the import statements inside a module.

The logic is based on some assumptions:

  • bare imports, meaning things are like import "polyfill" are not safe, meaning they have side effects;
  • the rest of the imports are safe, meaning they are side effects free;

The feature is behind a feature flag.

Test Plan

I created few tests, where we handle different statements, line breaks and cases of premature ending of the statements.

@ematipico ematipico temporarily deployed to aws April 28, 2022 09:03 Inactive
@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12391 12391 0
Failed 3866 3866 0
Panics 0 0 0
Coverage 76.22% 76.22% 0.00%

@ematipico ematipico marked this pull request as ready for review April 28, 2022 09:07
@ematipico ematipico force-pushed the feature/unstable-sort-imports branch from 785d1b5 to 0b8b709 Compare April 28, 2022 09:07
@ematipico ematipico temporarily deployed to aws April 28, 2022 09:07 Inactive
@github-actions
Copy link

github-actions bot commented Apr 28, 2022

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 28, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 856035b
Status: ✅  Deploy successful!
Preview URL: https://df893d28.tools-8rn.pages.dev

View logs

@ematipico ematipico force-pushed the feature/unstable-sort-imports branch from 0b8b709 to 856035b Compare April 28, 2022 10:31
@ematipico ematipico temporarily deployed to aws April 28, 2022 10:32 Inactive
@MichaReiser
Copy link
Contributor

We'll need to figure out if we indeed want the sorting to be part of the formatter or if it's rather something that should happen inside of the linter. The main issue with doing it inside of the formatter is that it isn't a 100% safe operation, it can change the semantic of the program and this may not be something that the formatter should do.

However, we can provide this as a refactor and allow users to (maybe even by default) opt in to run the sorting on safe.

@yassere yassere added the PR: on hold A PR that needs some upstream work before getting merged. label Jun 28, 2022
@ematipico ematipico closed this Jul 16, 2022
@ematipico ematipico deleted the feature/unstable-sort-imports branch July 16, 2022 14:32
@MichaReiser MichaReiser mentioned this pull request Oct 20, 2022
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: on hold A PR that needs some upstream work before getting merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants