Skip to content

feat(ordering): add support for insertInto option #371

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

feat(ordering): add support for insertInto option #371

wants to merge 3 commits into from

Conversation

brophdawg11
Copy link

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

The issue being addressed is describe in #370. This adds a new insertInto option that is optional and backwards compatible. Specifying a CSS selector allows the user to specify a DOM node into which the async-loaded <link> tags should be inserted.

Breaking Changes

n/a, changes are backwards compatible and will continue to insert <link> tags into the <head> if no insertInto option is specified

Additional Info

This is a different approach that solves (I think) the same problem as #331, in a more customizable and backwards-compatible way.

This is also seemingly related to #328.

@jsf-clabot
Copy link

jsf-clabot commented Apr 1, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

alexander-akait
alexander-akait reviewed Apr 1, 2019
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

I think better implement ability to declare own function with insert logic, it is allow to insert something in Shadow Dom or when you need more complex logic we do same in style loader in next major release

@brophdawg11
Copy link
Author

@evilebottnawi So just do something like .toString() on the incoming function to pass it through to the bundled output? Is that safe for all function types? I checked quickly, and this does evaluate properly in console:

(function foo() { console.log('hello world!'); }).toString()
// "function foo() { console.log('hello world!'); }"

If so, I can make that update

@alexander-akait
Copy link
Member

alexander-akait commented Apr 3, 2019 •
edited
Loading

@brophdawg11 yes, we do same for style-loader and no problem, also it is allow to write custom logic and it is more generic solution, thanks for PR

@brophdawg11
feat(ordering): add support for insertInto option
e0ec27d 
This adds an new `insertInto` option that is optional and backards-compatible.  Specifying a CSS selector allows the user to specify a DOM node into which the async-loaded <link> tags should be inserted.

Closes webpack#370, related to webpack#328,webpack#331
@brophdawg11
feat: convert insertInto option to be a function
56dabe7
@brophdawg11
docs: add docs for insertInto option
9a3328c
@brophdawg11
Copy link
Author

@evilebottnawi Sorry for the delay - I just updated this to take a function instead of a selector based on your comment above

@brophdawg11 brophdawg11 mentioned this pull request Jun 27, 2019
Closed
@ScriptedAlchemy ScriptedAlchemy mentioned this pull request Jul 30, 2019
Closed
@philipp-spiess
Copy link

style-loader recently made the custom insertion function a bit more generic by not requiring it to return a parent to insert the style into but instead allow the user to handle the DOM attachment on their own: https://github.com/webpack-contrib/style-loader/pull/413/files

I think it would be great if we can have the same behavior for mini-css-extract-plugin. What do you think?

@giuseppeg giuseppeg mentioned this pull request Oct 31, 2019
Closed
@giuseppeg
Copy link

👋 folks, I put up a similar PR to introduce the insert option as per @evilebottnawi's request #459 The API is the same as style-loader and a bit more flexible. @brophdawg11 I hope that's fine with you (I don't mean to diminishing your good work!)

@alexander-akait
Copy link
Member

Close in favor #459

@brophdawg11
Copy link
Author

Yep - thanks @giuseppeg! I agree this is more flexible and better aligns with style-loader's API. I was able to switch back to the normal package (from my fork) and use insert without issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexander-akait alexander-akait alexander-akait left review comments

+1 more reviewer

@ScriptedAlchemy ScriptedAlchemy ScriptedAlchemy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@brophdawg11 @jsf-clabot @alexander-akait @philipp-spiess @giuseppeg @ScriptedAlchemy