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.

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

PoC integration between browser and AMD modules #1533

Closed
wants to merge 1 commit into from
Closed

PoC integration between browser and AMD modules #1533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion modules/apps/frontend-taglib/frontend-taglib-clay-sample-web/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"name": "frontend-taglib-clay-sample-web",
"private": true,
"scripts": {
"build": "liferay-npm-scripts build",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm turning off any build here because I don't want babel or the bundler to rewrite anything.

"checkFormat": "liferay-npm-scripts check",
"format": "liferay-npm-scripts fix"
},
Expand Down
9 changes: 9 additions & 0 deletions ...aglib/frontend-taglib-clay-sample-web/src/main/resources/META-INF/resources/_amd/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const promise = new Promise((resolve, reject) => {
Liferay.Loader.require(
'liferay!frontend-js-react-web$react@16.12.0/index',
resolve,
reject
);
});

export default await promise;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a special module that would be created by the build system automagically. It provides a bridge from browser modules to AMD ones, i.e, lets index.js import react as if it was local to the project, but then it is diverted to the AMD loader transparently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The export await syntax is pure gold because it lets interchange of sync and async imports/exports without any need to change the consumer code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

browser modules to AMD ones

Any idea what the cost would be to migrate our AMD modules into browser modules? Or is this just a silly thought?

Copy link
Collaborator Author

@izaera izaera Oct 5, 2021 •
edited

Choose a reason for hiding this comment

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

In theory we would be able without touching anything in the source code. We would need a tool and some configuration files to specify where to find things.

However, we would need to do a PoC for that too, because the devil is in the details, and it's not the first time (in this context) we think something is easy, then we discover it is much more complex or even impossible (see what happened with webpack federation 😞 ).

5 changes: 5 additions & 0 deletions ...end-taglib/frontend-taglib-clay-sample-web/src/main/resources/META-INF/resources/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import holi from './js/holi.js';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Import local module holi.js synchronously

import react from './_amd/react.js';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Import third party module react synchronously.

Note that this is a trick for the PoC, but in the original source code we would have something like import react from 'react'; and then a configuration to tell the bundler to remap that react to something (in this case an AMD module, in other case it could be a local browser module inside node_modules or in any other OSGi bundle).

Copy link

Choose a reason for hiding this comment

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

It looks interesting :)

Screen Shot 2021-10-04 at 10 53 51

Mixing amd and esm modules will not cause a race condition?

Does the browser do these imports sequentially?

If not, I think the amd loader can be slower(I'm guessing, yes) and the browser can load assets in esm faster and a dependency of an esm can be the currently-loading amd loader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no race condition because we would use export await in the AMD-browser bridge module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the browser do these imports sequentially?

I haven't checked it, but I guess it doesn't. It can simply create the dep graph, fetch the files, and visit the graph in parallel (respecting deps, of course) instantiating and resolving modules. Much in the same way we do in AMD loader.

However, I haven't done any performance test yet. And I'm not 100% sure if deploying npm packages without bundling will be performant, because I don't know if the browser is going to download the modules one by one or in parallel (using HTTP 2 or something like that). This is something we need to explore before committing to any design.


holi();
document.getElementById('perico').innerHTML = `React instance: ${react}`;
3 changes: 3 additions & 0 deletions ...d-taglib/frontend-taglib-clay-sample-web/src/main/resources/META-INF/resources/js/holi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function holi() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would be a regular browser module.

Note that this is put inside the JAR verbatim: we don't need to build anything.

alert('holi!');
}
23 changes: 2 additions & 21 deletions ...end-taglib/frontend-taglib-clay-sample-web/src/main/resources/META-INF/resources/view.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,6 @@

<%@ include file="/init.jsp" %>

<liferay-ui:tabs
names="Alerts,Badges,Buttons,Cards,Dropdowns,Form Elements,Icons,Labels,Links,Management Toolbars,Navigation Bars,Pagination Bars,Progress Bars,Stickers"
refresh="<%= false %>"
>
<div id="perico"></div>

<%
String[] sections = {"alerts", "badges", "buttons", "cards", "dropdowns", "form_elements", "icons", "labels", "links", "management_toolbars", "navigation_bars", "pagination_bars", "progress_bars", "stickers"};

for (int i = 0; i < sections.length; i++) {
%>

<liferay-ui:section>
<clay:container-fluid>
<liferay-util:include page='<%= "/partials/" + sections[i] + ".jsp" %>' servletContext="<%= application %>" />
</clay:container-fluid>
</liferay-ui:section>

<%
}
%>

</liferay-ui:tabs>
<script src="/o/frontend-taglib-clay-sample-web/index.js" type="module" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how you bootstrap a browser module.

It is the equivalent of <aui:script require="<%= npmResolvedPackageName + ...%>">

In the future, we could make a new tag to avoid using <aui:script> to be able to make our own magic before printing the <script> tag to the HTML, but it would be straightforward, most probably.

Copy link

Choose a reason for hiding this comment

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

to make our own magic before printing

By magic you mean pointing to a valid resolved path(with version of the module and a path to /o/) like the generated NPMResolver ones?

Copy link
Collaborator Author

@izaera izaera Oct 5, 2021 •
edited

Choose a reason for hiding this comment

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

By magic I meant "anything we want to do with it, now or in the future". So I'm proposing to have our own taglib just for the sake of having an extra layer between the JSP and the actual <script> tag.

I don't know if in this browser modules solution we would need to have version numbers or not 🤔 In this case, the canonical names of modules contain the Web-ContextPath of the OSGi bundle containing them, and that has to be unique per OSGi bundle version, so maybe we can switch from npm versions to OSGi locations to designate which npm modules we want to use. It's something that we should investigate.

Most probably, we would want to do things like:

  • <new:script-tag import="/index.js"/> to import local (contained in the OSGi bundle contanining the JSP's web context) modules.
  • <new:script-tag import="react" context="frontend-js-react-web" /> to import a module contained in a different web context (if we wanted to allow for it).

Or, altenatively, we could have some way to configure mappings from logical package names to web context + name which are then applied when building. So, doing something like:

<new:script-tag import="react" /> in your code, then having a file where it says that react has to be loaded from frontend-js-react-web

Nothing is yet decided, but those are the possibilities I have currently in mind...

Copy link

@julien julien Oct 5, 2021 •
edited

Choose a reason for hiding this comment

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

This is very interesting @izaera, I also think that having this tag would probably make things easier. Can't wait to see the final result

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't wait to see the final result

Well, you will have too, because this is only a brainstorming, nothing that we have decided or planned yet 😅

Copy link

Choose a reason for hiding this comment

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