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

refactor: remove module aliases #4393

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Feb 9, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #4393 (abebdfe) into master (60706c3) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4393   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files         204      205    +1     
  Lines        7325     7326    +1     
  Branches     2081     2081           
=======================================
+ Hits         7232     7233    +1     
  Misses         34       34           
  Partials       59       59           
Impacted Files Coverage Δ
cli/cli.ts 71.42% <0.00%> (ø)
src/rollup/rollup.ts 100.00% <ø> (ø)
src/utils/PluginContext.ts 100.00% <ø> (ø)
cli/help.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60706c3...abebdfe. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Please have a look at the failing tests

@lukastaegert
Copy link
Member

Ah sorry, missed that this is a draft PR

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Feb 10, 2022

Ah sorry, missed that this is a draft PR

no worries. yeah, it's not quite there yet. the build currently works, if one kicks off node ./scripts/generate-help-module.js manually. it's working so far, but there is one more kink I noticed which does not belong there:

import { relative } from '../browser/path';
.

it's part of the preliminary work to make rollup self-hosted - although one could argue it already kind of is, but with an older version of itself, which came up here: #4219 (comment) .

I think it would make development and debugging, and therefor hopefully contributions, easier, without going through the rollup process. it would also be easier to run any performance related tools on top of the original code (typescript minus the stripped types) if one measures perf essentially against itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants