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

feat: Call namespace error as a warning #3475

Merged
merged 3 commits into from Mar 31, 2020
Merged

feat: Call namespace error as a warning #3475

merged 3 commits into from Mar 31, 2020

Conversation

@guybedford
Copy link
Contributor

@guybedford guybedford commented Mar 31, 2020

This converts the error when a namespace is called into a warning.

The reason being that there may exist valid runtime paths through the code that do not trigger the error, despite it being invalid. So allowing the build to proceed can allow valid runtime code to work despite the underlying "typing issue".

Alternatively it might be worth considering a strict / loose mode for Rollup "typing errors" so far as it cares about module semantics, but I actually think it would be better to treat Rollup as allowing anything to execute which would execute in a JS engine, since it is not a type system.

Landing as-is seems sensible to me, but I would be open to suggestions too.

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

@guybedford
Copy link
Contributor Author

@guybedford guybedford commented Mar 31, 2020

Again I have no idea why there are these trailing commas added or how to change that in the git hooks.

@rollup-bot
Copy link
Collaborator

@rollup-bot rollup-bot commented Mar 31, 2020

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#call-ns-warn

or load it into the REPL:
https://rollupjs.org/repl/?circleci=10262

@codecov
Copy link

@codecov codecov bot commented Mar 31, 2020

Codecov Report

Merging #3475 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3475      +/-   ##
==========================================
+ Coverage   95.81%   95.84%   +0.03%     
==========================================
  Files         174      174              
  Lines        5898     5898              
  Branches     1736     1736              
==========================================
+ Hits         5651     5653       +2     
+ Misses        127      126       -1     
+ Partials      120      119       -1     
Impacted Files Coverage Δ
src/ast/nodes/CallExpression.ts 95.41% <100.00%> (ø)
src/ast/nodes/TaggedTemplateExpression.ts 81.81% <100.00%> (+18.18%) ⬆️

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 5eb01fb...acf0777. Read the comment docs.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 31, 2020

Again I have no idea why there are these trailing commas added or how to change that in the git hooks

It's the new default of prettier@2. I will add a setting to change that.

Copy link
Member

@lukastaegert lukastaegert left a comment

Sounds good and I agree with the reasoning. I added tagged template literals to the tests as well to also cover this case in tests.

@lukastaegert lukastaegert merged commit c69c41b into master Mar 31, 2020
9 checks passed
@lukastaegert lukastaegert deleted the call-ns-warn branch Mar 31, 2020
@guybedford
Copy link
Contributor Author

@guybedford guybedford commented Mar 31, 2020

Thanks for the quick review!

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

Successfully merging this pull request may close these issues.

None yet

3 participants