Skip to content

build: don't show import path if there is no primary export #22804

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

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

crisbeto
Copy link
Member

Currently we show something like import {} from '@angular/cdk/coercion' for imports that don't have a primary export. These changes remove the line if there is no primary export.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent docs This issue is related to documentation merge safe target: patch This PR is targeted for the next patch release labels May 26, 2021
@crisbeto crisbeto requested a review from devversion May 26, 2021 06:30
@crisbeto crisbeto requested a review from a team as a code owner May 26, 2021 06:30
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 26, 2021
@devversion
Copy link
Member

I think this was intentionally displayed like that. Even if there is no primary export (which is usually a module or a test harness class), we'd want to show how the import to this entry-point would look like IMO.

@crisbeto
Copy link
Member Author

The current behavior just looks broken IMO since you aren't importing anything. Alternatively it can be changed to something like import * as coercion from '@angular/cdk/coercion';, but I don't know if we have the name for the import alias.

@crisbeto crisbeto requested a review from jelbourn May 26, 2021 15:20
@devversion
Copy link
Member

Yeah, let's see what others think 👍

@josephperrott josephperrott removed the request for review from a team June 1, 2021 17:25
@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2021

What if we make it something like

Import symbols from `@angular/cdk/coercion`

@crisbeto
Copy link
Member Author

crisbeto commented Jun 2, 2021

That's a good idea, but my only concern is that it would still be styled as code in the docs and people might think that it's valid syntax.

@jelbourn
Copy link
Member

jelbourn commented Jun 2, 2021

You could do something like

  {%- if doc.primaryExportName -%}
    <p>
      Import symbols from 
      <code  class="docs-api-module-import">
        import {{$ doc.primaryExportName $}} from '{$ doc.moduleImportPath $}';
      </code>
    </p>
  {%- endif -%}

(might require some tweaking, but with that general idea)

@crisbeto
Copy link
Member Author

crisbeto commented Jun 2, 2021

I don't follow how that would help. The problem this is trying to fix is when a doc.primaryExportName doesn't exist.

@devversion
Copy link
Member

devversion commented Jun 2, 2021

I was thinking @jelbourn meant something around that (even though the snippet shows something different though):

// If there a primary export, we show (highlighted)
import {<primaryExport>} from '@angular/cdk/coercion';
// If there is no primary export, we show (without highlighting)
// Using <code> for the import so that it can be copied better
Import symbols from <code>@angular/cdk/coercion</code>

Currently we show something like `import {} from '@angular/cdk/coercion'` for imports that don't have a primary export. These changes remove the line if there is no primary export.
@crisbeto crisbeto force-pushed the primary-export-docs branch from 9ec8b61 to 8745b9e Compare June 2, 2021 18:09
@crisbeto
Copy link
Member Author

crisbeto commented Jun 2, 2021

Thank you for the explanation, that makes sense. I've pushed the change.

@jelbourn
Copy link
Member

jelbourn commented Jun 2, 2021

Yeah, that's what I meant, Paul is just smarter than me

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

lgtm

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jun 2, 2021
@mmalerba mmalerba merged commit e2633d1 into angular:master Jun 2, 2021
mmalerba pushed a commit that referenced this pull request Jun 2, 2021
Currently we show something like `import {} from '@angular/cdk/coercion'` for imports that don't have a primary export. These changes remove the line if there is no primary export.

(cherry picked from commit e2633d1)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants