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

Add asset info to library guide #34217

Closed
wants to merge 2 commits into from

Conversation

jbogarthyde
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

No mention in docs of possible ways to add assets to a library

Issue Number: #33141

What is the new behavior?

Adds a section on assets to https://angular.io/guide/creating-libraries

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@jbogarthyde jbogarthyde added type: bug/fix comp: docs effort2: days freq2: medium aio: preview target: patch This PR is targeted for the next patch release risk: low state: needs eng review Needs technical review and approval from engineering team labels Dec 3, 2019
@jbogarthyde jbogarthyde requested a review from a team as a code owner December 3, 2019 19:55
@jbogarthyde jbogarthyde self-assigned this Dec 3, 2019
@ngbot ngbot bot added this to the Backlog milestone Dec 3, 2019
@jbogarthyde
Copy link
Contributor Author

@Splaktar Please take a look and help me fill this in properly.

@jbogarthyde jbogarthyde added this to In Progress in docs Dec 3, 2019
@mary-poppins
Copy link

You can preview e95c7cf at https://pr34217-e95c7cf.ngbuilds.io/.

@@ -156,6 +156,27 @@ List all the peer dependencies that your library uses in the workspace TypeScrip

This mapping ensures that your library always loads the local copies of the modules it needs.

{@a lib-assets}

## Including assets with libraries
Copy link
Contributor

Choose a reason for hiding this comment

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

From devrel perspective how do we usually communicate something like this?

That being, while this is documented it’s not something that we support.

//cc @mgechev

Copy link
Member

Choose a reason for hiding this comment

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

In what sense is this "not supported"? You just want to call out that scss-bundle is not owned and maintained by the Angular team?

Copy link
Member

Choose a reason for hiding this comment

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

We can add a warning box stating that this is not supported by the team, but an approach provided by a third-party library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like that, my point is that using a package might cause undefined behaviour, example unless changed using this package will opt you out of using sass and will force you to use the legacy node-sass since it has direct dependency on node-sass. This will have a effect on all workspace projects which might break currently working project.

Ps: will need to confirm if they still use node sass.

Also, afaik previously we reframing from adding certain documents in aio for “unsupported” reasons, such as HMR, bootstrap etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should revisit and add documents such as how to configure HMR etc...

@Splaktar Splaktar mentioned this pull request Dec 3, 2019
14 tasks
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
aio/content/guide/creating-libraries.md Outdated Show resolved Hide resolved
@mary-poppins
Copy link

You can preview 18a1495 at https://pr34217-18a1495.ngbuilds.io/.

@jbogarthyde
Copy link
Contributor Author

@Splaktar @jelbourn Now that Alan has fixed ng-packagr , what should the "Creating Libs" page say about assets?
Since so many people have been having trouble with it for so long, is seems like we should say something, if only to reassure them that all they need to do is update to the new release.

@jelbourn
Copy link
Member

jelbourn commented Dec 4, 2019

Seems like documenting the ng-packagr API to do this would be the right thing to do, maybe adding a note like "Available as of version xxx"

@mary-poppins
Copy link

You can preview dae1e02 at https://pr34217-dae1e02.ngbuilds.io/.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@mary-poppins
Copy link

You can preview 7145a1b at https://pr34217-7145a1b.ngbuilds.io/.

kara pushed a commit that referenced this pull request Feb 7, 2020
@kara kara closed this in 26b17ae Feb 7, 2020
@jbogarthyde jbogarthyde deleted the jb-lib-assets branch February 10, 2020 17:17
@kapunahelewong kapunahelewong moved this from In Progress to Done in docs Feb 10, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 17, 2020
@Splaktar
Copy link
Member

It looks like no one was able to respond to my concerns in #34217 (comment). I don't know if they just got lost in the shuffle or if they were deemed invalid.

@jbogarthyde
Copy link
Contributor Author

@Splaktar Can you be more specific? What would you like to change? We did move the section on asset management to a different part of the document, I thought that was in response to your comment.

@Splaktar
Copy link
Member

@jbogarthyde I'll post my comment here again since it seems to have gotten lost (no action taken and no response):


I think that this section on publishing apps with additional assets is related to "publishing your library". This is the step of library development where library authors run into the issue of needing to copy assets during build time into their production bundle for publishing on NPM. This often happens when an already published library wants to add theming mixins to their published library.

I think adding this to a separate "building your library" section could work, but currently the "getting started" section covers ng build my-lib and the "publishing your library" section covers ng build my-lib --prod.

The current hierarchy doesn't seem to make sense
Screen Shot 2020-02-03 at 14 02 13

It doesn't seem like "Managing library assets with ng-packagr" should be nested under the "Using your own library in apps" section.


I'm still seeing this same structure on https://next.angular.io/guide/creating-libraries.

If we want to keep this section, "Managing library assets with ng-packagr", at the end of the page, then I think that it needs to not be indented/nested under "Using your own library in apps".

@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 Mar 27, 2020
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 aio: preview cla: yes effort2: days freq2: medium risk: low target: patch This PR is targeted for the next patch release type: bug/fix
Projects
docs
Done
Development

Successfully merging this pull request may close these issues.

None yet

10 participants