Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Fragment server with multiple fragment-script / stylesheet resources? #136

Closed
iilei opened this issue Mar 29, 2017 · 10 comments
Closed

Fragment server with multiple fragment-script / stylesheet resources? #136

iilei opened this issue Mar 29, 2017 · 10 comments

Comments

@iilei
Copy link
Contributor

iilei commented Mar 29, 2017

Would you appreciate a PR that enables multiple fragment-script / stylesheet resources?

possibly via "name" insted of "rel" property?

@vigneshshanmugam
Copy link
Collaborator

@iilei
Copy link
Contributor Author

iilei commented Mar 29, 2017

I am talking about a different use case. I'd like to send something among the lines of the following:

Response Headers:
Link:<http://localhost:8042/js/my-app-libs.js>; rel="fragment-script",<http://localhost:8042/js/my-app-main.js>; rel="fragment-script"

The way the headers are parsed in lib/fragment.js, the last definition of a Header Link with rel="(stylesheet|fragment-script)" wins: my-app-main.js in this example. (not ideal imho)

@vigneshshanmugam
Copy link
Collaborator

@iilei Ahh, Got you. It was a conscious decision to not allow multiple link headers in the response for performance reasons. Fragment Devs could easily bloat the whole page by sending the libs and vendors and whatever chunked scripts as part of the headers.

The proper way of doing this would be to do code splitting and send the only main-app.js that has the necessary modules for the initial state of the application and lazy load all the code splitted modules from the main-app.js

@iilei
Copy link
Contributor Author

iilei commented Mar 31, 2017

Thanks for your response. I get your point. I have made pretty good progress on my fork adding the desired behavior, so if you are interested anyways, I'd be happy to file a PR.

The change would be non breaking and could also be controlled via some default config option (e.g. {maxScriptLinks : 1, maxStyleLinks : 1} or so).

@dazld
Copy link
Contributor

dazld commented Apr 11, 2017

Thinking out loud - @vigneshshanmugam and @iilei please do tell me to keep quiet if not helpful - but here's a couple of observations:

  • if there is a resource that has already been served, then it shouldn't be included again. I think this provides a number of benefits, in that fragments can be explicit about what they need in terms of resources, without worrying about including the same file multiple times. it boosts resilience and autonomy for fragments as well, in that they don't need to coordinate with other fragments about shared resources, and can just assume that tailor is doing the right thing. I think this is valuable.

  • the max configuration option feels a little artificial and boilerplatey. It's tempting to have some kind of programmatic limit, but this shouldn't be an alternative to teams performing due diligence and following good practice. If there is a situation where you cannot trust that someone isn't going to put 1000 script files in their fragment, then just chopping it off at an arbitrary limit is the least of your worries.

Would welcome your thoughts on the above!

@dazld
Copy link
Contributor

dazld commented Apr 11, 2017

just in case it's not clear as well - I'm a fan of this multiple resources feature :)

@iilei
Copy link
Contributor Author

iilei commented May 12, 2017

If there is a situation where you cannot trust that someone isn't going to put 1000 script files in their fragment, then just chopping it off at an arbitrary limit is the least of your worries.

@dazld Allowing it to be undefined would spoil the performance. Hence the config option.

vigneshshanmugam pushed a commit that referenced this issue Jun 1, 2017
* first defined Link wins, while others are dropped (breaking change, before it was the last Link)
* Allow opt-in for multiple Header Link-rels `{ maxAssetsLinks: 1 }
* Add example for multiple Link Headers
* issue #136 [feature]
@vigneshshanmugam
Copy link
Collaborator

Addressed via #140

@duff907
Copy link

duff907 commented Jun 6, 2017

@vigneshshanmugam This looks like a useful change, could this be released to the npm module?

@vigneshshanmugam
Copy link
Collaborator

This breaks the performance hooks of fragments, Will release the new version this week after fixing the hooks.

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

No branches or pull requests

4 participants