engine-server/template-compiler: remove parse5 utils

[43081j]

Details

I had a branch hanging around waiting for inikulin/parse5#996, which upgraded parse5 in this repo to 7.x

While waiting, this was partially done in #3602 (partially because we're still waiting for the parse5 update, just using ts-ignore meanwhile).

One thing that branch didn't have, which would be good to still sort: removal of the parse5 utils in LWC.

Basically, LWC ships its own parse5 utils for things like quick access to tree adapter types, etc.

All of this has been available for a while in a @parse5/tools package.

So i've rebased my original PR back onto master, which has left us with the migration to @parse5/tools.

Failing build

Currently, it won't build as @parse5/tools is ESM-only and our target appears to be commonjs. How did you solve this elsewhere? @nolanlawson i saw you did some stuff around this for parse5 itself, could you point me in the right direction?

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ✅ No, it does not introduce an observable change.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @43081j to sign the Salesforce Inc. Contributor License Agreement.

[43081j]

i did sign the CLA btw but seems to have not picked it up 🤔

[nolanlawson]

@43081j For ESM dependencies, we are currently pre-bundling those into our own commonjs/ESM outputs, to avoid passing on the "type": "module" requirement to our own consumers.

To bundle up @parse5/utils, you will need to add it to the allowlist here:

lwc/scripts/rollup/rollup.config.js

Lines 133 to 138 in 1c1687e

	nodeResolve({
	// These are the devDeps that may be inlined into the dist/ bundles
	// These include packages owned by us (LWC, observable-membrane), as well as parse5
	// and its single dependency, which are bundled because it makes it simpler to distribute
	resolveOnly: [/^@lwc\//, 'observable-membrane', /^parse5($|\/)/, 'entities'],
	}),

It will also need to be added to the devDependencies in @lwc/template-compiler/package.json:

lwc/packages/@lwc/template-compiler/package.json

Lines 53 to 57 in 1c1687e

	"devDependencies": {
	"@types/estree": "1.0.2",
	"@types/he": "^1.2.1",
	"@types/source-map": "0.5.7"
	}

If you do this, then the dep should be bundled into packages/@lwc/template-compiler/dist/index.cjs.js rather than require()d.

I just realized though that we missed this step in #3602; we'll open up a separate PR to fix that.

As for the CLA – you may have to push an empty commit to the branch to re-trigger the CLA check. 🙂

[nolanlawson]

Fixing the devDeps issue here, you may have some merge conflicts after it gets merged: #3786

[nolanlawson]

This LGTM except for the merge conflict/devDeps issue that needs to be resolved, but I would like @divmain to take a look too.

Thank you so much for the PR!! 🙇

[43081j]

no worries, i'll update per the comments 👍

LWC is one of the last few major projects i've been updating parse5 and moving to p5 tools in. so will be good to get it done!

[43081j]

@nolanlawson i made the changes but, locally at least, build fails still when trying to build the playground:

Error [ERR_REQUIRE_ESM]: require() of ES Module /lwc/node_modules/@parse5/tools/lib/main.js from /lwc/packages/@lwc/template-compiler/dist/index.cjs.js not supported.

[nolanlawson]

@43081j When I build your branch locally, it works fine for me. I can also see that packages/@lwc/template-compiler/dist/index.cjs.js correctly has parse5 and parse5-utils inlined.

I will kick off a CI test of your code!

[divmain]

This is fantastic! Thanks again for the contribution and for reading my mind in the parse5 PR!

[divmain]

Suggested change

	import {DocumentFragment} from '@parse5/tools';
	import { DocumentFragment } from '@parse5/tools';

[43081j]

@nolanlawson it was some nx caching stuff, yarn build --skip-nx-cache sorted it (i guess the rollup file change didn't invalidate the cache)

@divmain no worries, thanks for sorting the parse5 upgrade too. once the new version is published, we can remove the ts-ignore flags too

[43081j]

the build failure seems like it has transformed to cjs but kept the export * 🤔

not too sure whats going on there

[divmain]

@43081j , feel free to pull in commit 728fb57. This should resolve the issue with Jest. We may follow up and make changes later, but don't worry about that for now.

You'll also find a single failing test related to your changes :)

[43081j]

interesting, do you know why that solves the problem? why doesn't ts-jest figure out ESM?

are we basically working around ts-jest by using babel on esm dependencies separately?

sorry for so many questions, just making sure i understand the root of the problem

---

as for the failing test... its an awkward one.

parse5 parses this as a template node (i.e. it has content):

<template>
  foo
</template>

however, it parses this an element with the nodename/tagname template but no content (so it isn't a template element, its an element named template... 😬 ):

<svg>
  <template>
    foo
  </template>
</svg>

i suspect this is because a template tag in svg is nonsense as far as the spec is concerned (it is a HTML element, doesn't exist in SVG).

i've worked around it by only using content if it actually has child nodes.

[nolanlawson]

why doesn't ts-jest figure out ESM?

I think it's that Jest just has problems with Native Node ESM (.mjs/"type": "module"): https://gist.github.com/rstacruz/511f43265de4939f6ca729a3df7b001c

[divmain]

Also, ts-jest doesn't operate on .js files (which is what is found in the published version of @parse5/tools), nor does it operate by default on node_modules. Another way to resolve this may have been to try to configure ts-jest differently, but that wasn't what occurred to me at the time.

[nolanlawson]

/nucleus test

[nolanlawson]

Thank you again!