izaera / fat-jar-sample

This a WIP to implement the fat JAR feature
0 stars 0 forks source link

General Feedback #1

Open bryceosterhaus opened 3 years ago

bryceosterhaus commented 3 years ago

I wasn't sure the best way to provide feedback and thought maybe an issue was best... Anyways, I think this is a really good direction and is giving me hope for this fat jar idea. I had a few notes and thoughts on the implementation but overall I think it's looking pretty good.


  1. Only a single dependency? https://github.com/izaera/fat-jar-sample/blob/0c952e9a780312c5acf51610ca9967f847901458/packages/fat-test/package.json#L6-L7

Would it be possible to merge dependencies and the bundler script could be called from that other package?

  1. Naming and version I'm not totally opposed to your current naming scheme, but my one concern would be that we might be telling someone "You need liferay-7.3-GA2 at version 7.2.0". which might be confusing for people.

  2. Specify as a normal dependency Since we are including dependencies that will end up on the client, can we call this a normal dependency rather than dev? I would be worried that developers might see "devDependency" and feel like they have a dependency-less project when they really don't.

  3. Is the preset necessary? https://github.com/izaera/fat-jar-sample/blob/0c952e9a780312c5acf51610ca9967f847901458/packages/fat-test/.npmbundlerrc#L1-L3

Can we infer the preset based on what dependency and version they specify in their package.json? This specification seems slightly redundant. Or is there a use case where we might have a dependency of liferay-7.4-GA1 and a preset of some other version?

  1. Whats this for again? https://github.com/izaera/fat-jar-sample/blob/0c952e9a780312c5acf51610ca9967f847901458/packages/fat-test/.npmbuildrc#L1-L5

I'm guessing this is boilerplate we already require but where does this come from again and why is it necessary?

  1. Small wrapper for dependency management and which scripts to call

One thought I had while reading through your commits was to create a light wrapper included with our main dependency(liferay-7.4). Calling this in build and deploy would determine whether bundler is used, if babel is used directly, if lnbs is used, etc. I may be oversimplifying some of this here but we may be able to hide some of our tooling. This would also allow us to have a little more flexibility with our bundler and not have to pack it with too many specifics for this fat jar use case.

izaera commented 3 years ago

1) Yes, I think we can make liferay-7.4-GA1 on liferay-npm-bundler and that should do it... We can leave the semver expression open so that if the user wants to force a newer version of the bundler he can.

2) Well, hopefully that doesn't happen and we only need 1.x versions... However, the version in the name is necessary, because if we code it inside the real version number (i.e. if we name the dependency liferay and use 7.4.1 as version number, we lose the possibility to release more versions in case anything goes wrong (because we have used the three numbers available). Also, we don't have any control over Liferay versioning scheme so, if it varies in the future we are in trouble. Even now we could be, because there's DXP in addition to CE.

A lateral thinking approach solution to having a version 7.2.1 for liferay-7.4-GA1 would be to simply skip 7.x versions, to avoid confussion :sweat_smile: . So we jump from 6.x to 8.x, for example. Or something similar...

3) :thinking: It probably makes more sense as a normal dep, since you are saying you are relying on Liferay 7.4 platform for your project to work and that's clearly a normal dep. But, on the other hand, that would make anyone depending on your project download the whole JS Toolkit and deps even if they didn't use them. TBH, I'm not sure about the implications of that...

I think we should think about this in depth, because it looks like it's an important aspect of the model we are defining.

4) Yes it is, but we can modify the bundler so that liferay-7.4-GA1 does something to make the bundler infer it.

5) Those are needed for two utility scripts in liferay-npm-build-support: translate and deploy. The translate one is used to invoke Microsoft Translation Services to maintain language keys and it is inside the JS Toolkit only because we put it there long ago, but it could be extracted, since it's self contained. Regarding deploy, we need it to copy the generated JAR to the Liferay deploy dir, but could be as well be done from outside the JS Toolkit.

In general you may think of liferay-npm-build-support as something similar to npm-scripts but living inside the JS Toolkit workspace for historical reasons.

6) I like embedding the script in the liferay-7.4-GA1 dependency for now. It looks simple and flexible, as you say. We always have time to make a new project in the future if needed.

So I'm going to work in a PR to try to include these things in the frontend-projects repo. That way we can start polishing this based on that PR.

javiergamarra commented 3 years ago

I like the approach. My feedback is pretty similar as Bryce's:

  1. I would merge those if possible.

    "liferay-npm-bundler": "^2.24.3",
    "liferay-7.4-GA1": "^1.0.0"
  2. About "liferay-7.4-GA1": "^1.0.0" I'm Ok with it. What would be the cases that force to use a new version like 1.0.1 or 1.1? I think Brian will like it because it follows the same idea as the target platform in Java and you have to remember/apply only 1 version, hopefully always 1.0.0.

  3. About devDependencies... It's true that it would be more correct specifying it in dependencies but I'm worried about the implications of that. People are always using our tools in wildly different ways and forcing them or assuming that they are going to use the bundler can bite us in the future. We have to think on it.

  4. Remove the preset! :P

I like the solution, with those changes I think it achieves Brian's goal perfectly.

izaera commented 3 years ago
  1. What would be the cases that force to use a new version like 1.0.1 or 1.1?

Probably only patches (third version number) if we do something wrong in the first release.

Also, it is possible that we may want to upgrade the build tool versions to force a minimum version greater than the one released in 1.0.0. For example, say we find a bug in liferay-npm-bundler@2.24.0 and we released portal-7.4.0-ga1 with liferay-npm-bunlder: ^2.24.0, it's possible that we may want to upgrade all platforms to liferay-npm-bunlder: ^2.24.1 so that new project don't fail because of the fixed bug.

  1. Specify as a normal dependency

IINM we will need to add everything as dependencies, not devDependencies, otherwise it won't be downloaded by npm and the user won't have them available :grimacing:

  1. Remove the preset! :P

That's https://github.com/liferay/liferay-frontend-projects/issues/542

izaera commented 3 years ago

Regarding :two:, I'm thinking that we could also want to implement a new feature in the bundler that needs modifications to the preset and, in that case, we would want to release a new minor version of the platform, since it embeds the preset or even a major one if it is a breaking change for already existing projects.

In other words, the target platform version number is a function of both:

  1. the portal state when it was released
  2. the build process configuration

1 is obviously constant since it is a released artifact, but 2 can evolve in time for the same target 1.

bryceosterhaus commented 3 years ago

if we name the dependency liferay and use 7.4.1 as version number, we lose the possibility to release more versions in case anything goes wrong

Ah that makes sense. liferay-7.4-GA1 makes sense then, although we should namespace it to avoid anyone from snagging a npm name before we get it. @liferay/7.4-GA1?

But, on the other hand, that would make anyone depending on your project download the whole JS Toolkit and deps even if they didn't use them. TBH, I'm not sure about the implications of that...

I think it's fine if they download the whole toolkit and dependencies. If they are using our package we can assume they are likely using our build process We wouldn't want them to get bundled into their module and sent to the client if they don't use it though. But I agree, we should think through this more carefully and fully analyze it before choosing one or the other.


As for everything else you noted, those sound reasonable to me and I think the direction of this tool makes. I'm excited to take form and try it out!