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

Update Rollup config #4282

Merged
merged 13 commits into from
May 31, 2022
Merged

Update Rollup config #4282

merged 13 commits into from
May 31, 2022

Conversation

slimbuck
Copy link
Member

@slimbuck slimbuck commented May 30, 2022

This PR replaces the specific build target configurations with a function which can construct the target config for any combination of build type (release, debug, profile, min) and module type (es5 and es6).

This means we can now generate profile, debug and minified es6 module builds too.

Also:

  • add the module build of profiler, debug and min to the npm package
  • generate inline sourcemap in debug build
  • add a few missing package scripts for build and watch

@slimbuck slimbuck requested a review from a team May 30, 2022 10:44
@slimbuck slimbuck self-assigned this May 30, 2022
@slimbuck
Copy link
Member Author

This PR means we now have 8 possible engine targets, since buildType=[release, debug, profile, min] and moduleType=[es5, es6].

Do we want to build them all when invoking npm run build?

For application development I specifically want playcanvas.dbg.mjs.

How do we go about adding all build targets to package.json?

@slimbuck
Copy link
Member Author

A simpler set of build targets to replace what we have now:

  • empty: build all 8 outputs
  • es5: all 4 es5 outputs
  • es6: all 4 es6 outputs
  • release: both release outputs
  • debug: both debug outputs
  • profile: both profile outputs
  • min: both min outputs

@slimbuck
Copy link
Member Author

Another question, do we add the new output files to the npm package? (I think yes).

@mvaligursky
Copy link
Contributor

it seems the simpler build targets build 2, 4 or 8 targets a time. I hope we still have one which builds a single target a time?
Also, just making sure that watch is available for those too.

@willeastcott
Copy link
Contributor

Yes, we add the new output files to the NPM package.
I'm wondering whether it's more accurate to say:

  • es5 -> cjs
  • es6 -> esm

@slimbuck
Copy link
Member Author

The rollup script now handles the following values for target:

  • build type: release, debug, profiler, min
  • module type: es5, es6
  • specific combination: release_es5, debug_es6 etc

An invalid target will result nothing being built.

@slimbuck
Copy link
Member Author

Also added general watch script, which can be used to watch everything, a subset or a specific target.

For example, to watch debug es6 build do:

target=debug_es6 npm run watch

The same holds for build script cc @mvaligursky.

@slimbuck
Copy link
Member Author

Yes, we add the new output files to the NPM package. I'm wondering whether it's more accurate to say:

  • es5 -> cjs
  • es6 -> esm

I'm not super familiar with this naming, I'd probably get confused. Is this really standard way of referring to es5 vs es6 output?

@willeastcott
Copy link
Contributor

esm would be ES Modules (using import/export). And cjs would be CommonJS (using modules.export). We don't have to change that here, but it's potentially a better naming...

@slimbuck
Copy link
Member Author

Finally:

  • publish the new output files to the npm package
  • add inline source maps in debug build

Sorry for the flurry of changes to the original PR.

It is now ready for review.

"build/playcanvas.dbg.js",
"build/playcanvas.prf.js",
"build/playcanvas.mjs",
"build/playcanvas.min.mjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue we shouldn't ship this. Devs will use the module in their own build process which will do its own bundling/minification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I agree it might have limited usefulness, but is that reason enough to remove it from the package? For example couldn't users in future load the mjs from the browser directly, in which case the min version might be useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

They could load the module build directly from a script tag but I don't see that as being a common use case. I guess there's no major harm to including it. But I might suggest removing it in future. 😄

@slimbuck slimbuck merged commit eac4ab2 into playcanvas:main May 31, 2022
@slimbuck slimbuck deleted the rollup-config branch May 31, 2022 12:06
@willeastcott
Copy link
Contributor

@slimbuck I have a feeling that this PR removed the optimization that would output both playcanvas.js and playcanvas.min.js from the same build target (that saved 10 or so seconds over building both targets separately). The same technique could also now be used for playcanvas.mjs and playcanvas.min.mjs.

@slimbuck
Copy link
Member Author

slimbuck commented Jul 4, 2022

@slimbuck I have a feeling that this PR removed the optimization that would output both playcanvas.js and playcanvas.min.js from the same build target (that saved 10 or so seconds over building both targets separately). The same technique could also now be used for playcanvas.mjs and playcanvas.min.mjs.

You are right. Release build always built both outputs.

So now, if you're after a single target it'll build a little quicker. But if you're after both targets then it'll build slower.

I've looked at the rollup code but haven't yet managed to figure out a straightforward way of accomplishing this. Will keep thinking.

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

Successfully merging this pull request may close these issues.

None yet

4 participants