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

Simplified ESM build #3704

Open
wants to merge 3 commits into
base: v2
Choose a base branch
from
Open

Simplified ESM build #3704

wants to merge 3 commits into from

Conversation

trusktr
Copy link

@trusktr trusktr commented Jul 5, 2023

Issue:

Continues from PR

To test this in an existing app, install Prism v2 like so:

npm install prismjs@github:trusktr/prism#simple-esm

To try an existing Prism v2 example along with a Node ESM test try this repo:

https://github.com/trusktr/prism-v2-test

npm install # this installs Prism from git, from this branch
npm test # this verifies that importing Prism in Node ESM works (we should perhaps move that test into this repo)

The gold standard today is that if a project is both as close to vanilla ESM as possible and importable in Node ESM, it is likely to work with today's build tools.

In particular, this change tries to stay as close to vanilla ESM as possible, by avoiding the use of package.json exports field (browsers do not read package.json, let along an exports field).

The example app above shows importing Prism without any configuration or build. In the worst case, if they wish to avoid putting ./node_modules directly in their import statement, they can create a <script type=importmap> element to map prismjs to the node_modules location.

All Mocha unit tests pass except two of them. I also removed the components.json build, which I think needs to be restored for when new languages are added (? I'm not sure yet), but now we'll ensure that we restore what is needed on this simplified foundation.


The mantras here are:

  • simple ES Module output that requires the least amount of tooling or configuration to import in vanilla ES Modules (namely, in a browser where this is most likely to be imported, where browser ESM is a subset of Node ESM).
  • we start with most vanilla ESM setup before starting to add any build steps, so that we're staying on the most compatible ESM path.

This is as simple as it can possibly get for browser ES Modules (apart from us also including a sample importmap that they can paste into their HTML, which we can do.

This is still WIP, we need to look at the components.json build requirement.



Comments in the diff have some more details.

trusktr and others added 2 commits June 29, 2023 18:32
The `prepare` script is the NPM-standard way in which outside-of-NPM packages can be build, for example with installed from a git location.

After merging this into the main branch (currently `master`), people will be able to do this:

```sh
npm install prismjs@github:PrismJS/prism
```

If they want to try it right now before v2 is merged (if this is merged into v2), they can do this:

```sh
npm install prismjs@github:PrismJS/prism#v2
```

Without this, such installations are not possible.

What NPM does during the `install` process is

- it clones the specified repo,
- checks out the specified git ref (f.e. `v2`),
- installs dependencies including dev dependencies,
- runs the `prepare` script if any (the `prepare` script will *always* have dev dependencies available to it),
- and finally packages the result in the same way as `npm pack` such that build outputs are included in the final package that gets installed into the user's node_modules

This would be a way for people to easily test v2 right now and provide feedback.
… structure, with type definitions and source maps, in a way that is as compatible with vanilla ES Modules as possible
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

No JS Changes

Generated by 🚫 dangerJS against 3486a8e

@@ -2,14 +2,16 @@
"name": "prismjs",
"version": "1.29.0",
"description": "Lightweight, robust, elegant syntax highlighting. A spin-off project from Dabblet.",
"main": "prism.js",
"type": "module",
"main": "dist/core/prism.js",
Copy link
Author

Choose a reason for hiding this comment

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

Opt into the modern Node ESM standard, and point to the main file to get Prism from (there was no more ./prism.js).

"style": "themes/prism.css",
"engines": {
"node": ">=14"
},
"scripts": {
"benchmark": "ts-node benchmark/benchmark.ts",
"build": "ts-node scripts/build.ts",
"build": "ts-node-esm scripts/build.ts && npm run tsc",
Copy link
Author

Choose a reason for hiding this comment

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

Running with ts-node-esm runs files as Node ESM, rather than CommonJS.

"style": "themes/prism.css",
"engines": {
"node": ">=14"
},
"scripts": {
"benchmark": "ts-node benchmark/benchmark.ts",
"build": "ts-node scripts/build.ts",
"build": "ts-node-esm scripts/build.ts && npm run tsc",
"prepare": "npm run build",
Copy link
Author

Choose a reason for hiding this comment

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

This is from #3699

@@ -23,7 +25,8 @@
"test:plugins": "ts-mocha tests/plugins/**/*.ts",
"test:runner": "ts-mocha tests/testrunner-tests.ts",
"test": "npm-run-all test:*",
"tsc": "tsc && tsc -p tests/tsconfig.json"
"typecheck": "tsc --noEmit && tsc -p tests/tsconfig.json --noEmit",
"tsc": "tsc"
Copy link
Author

Choose a reason for hiding this comment

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

We use a plain and simple tsc step to build dist/ output. The new typecheck script replaces the old tsc script for type checking without emitting output.

We should probably cover the other TS files as well, currently this covers only src/ and tests/.

@@ -15,7 +15,7 @@ module.exports = {
'eqeqeq': ['error', 'always', { 'null': 'ignore' }],

// imports
'import/extensions': ['warn', 'never'],
'import/extensions': ['warn', 'always'], // "always" is most compatible with default vanilla ES Modules
Copy link
Author

Choose a reason for hiding this comment

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

Having .js extensions, even if not to taste, is needed for the simplest ESM setup, and TypeScript officially recommends writing .js directly in TypeScript import statements for this purpose. So, we force it here so we don't mess up.

If we remove the .js extensions, the vanilla ESM example over at https://github.com/trusktr/prism-v2-test will break.

@@ -15,7 +15,7 @@ module.exports = {
'eqeqeq': ['error', 'always', { 'null': 'ignore' }],
Copy link
Author

@trusktr trusktr Jul 6, 2023

Choose a reason for hiding this comment

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

This file is the only remnant of CommonJS in the repo (apart from the dependencies which I haven't updated yet).

In the upcoming ESLint v9, the new "flat config" format will also be in Node ESM format, and we'll have to move ESLint config to ESM at that point.

@@ -0,0 +1,7 @@
{
"Xrequire": "@babel/register",
"Xexperimental-specifier-resolution=node": "",
Copy link
Author

Choose a reason for hiding this comment

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

  • cleanup

"Xrequire": "@babel/register",
"Xexperimental-specifier-resolution=node": "",
"node-option": [
"loader=ts-node/esm"
Copy link
Author

Choose a reason for hiding this comment

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

This makes ts-mocha run our files as Node ESM files.

import rollupTerser from '@rollup/plugin-terser';
import rollupTypescript from '@rollup/plugin-typescript';
// import rollupTerser from '@rollup/plugin-terser';
// import rollupTypescript from '@rollup/plugin-typescript';
Copy link
Author

@trusktr trusktr Jul 6, 2023

Choose a reason for hiding this comment

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

  • cleanup: delete unused code

Note how much less build code we need to maintain with this setup. But see the comment about components.json.

// all_languages_placeholder: () => Promise.resolve(languageIds),
// title_placeholder: async () => {
// const rawTitles = new Map<string, string>();
// for (const [id, entry] of Object.entries(components.languages)) {
Copy link
Author

Choose a reason for hiding this comment

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

  • todo: figure if/how to handle components.json. For now I simply disabled buildJS.

@@ -42,7 +44,7 @@ async function minifyCSS() {
for (const id of pluginIds) {
const file = path.join(SRC_DIR, `plugins/${id}/prism-${id}.css`);
if (fs.existsSync(file)) {
input[`plugins/prism-${id}.css`] = file;
input[`plugins/${id}/prism-${id}.css`] = file;
Copy link
Author

Choose a reason for hiding this comment

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

Here we output the .css files so they are in the same location in dist/ as they are in src/.

Less confusion: For anything in src/**/*, just replace src/ with dist/ to arrive at the output path. Easier mental model.


const __dirname = path.dirname(fileURLToPath(import.meta.url));
Copy link
Author

Choose a reason for hiding this comment

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

__dirname is not available in Node ESM files. Instead we have import.meta.url from which to base path handling.

@@ -25,35 +25,31 @@
// "moduleDetection": "auto", /* Control what method is used to detect module-format JS files. */

/* Modules */
"module": "commonjs", /* Specify what module code is generated. */
"module": "ESNext", /* Specify what module code is generated. */
Copy link
Author

Choose a reason for hiding this comment

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

Switch to ESM output format. All JS engines support top-level await (esnext).

// "rootDir": "./", /* Specify the root folder within your source files. */
// "moduleResolution": "node", /* Specify how TypeScript looks up a file from a given module specifier. */
"moduleResolution": "node", /* Specify how TypeScript looks up a file from a given module specifier. */
Copy link
Author

Choose a reason for hiding this comment

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

This is not part of vanilla ESM, but it is very common to have this set to node even for vanilla web projects, using import maps as needed, because this is the default and simplest way in which type definitions flow into code in the ESM format (TS automatically grabs types from node_modules, which is the most common way people currently install libraries).

This covers non-relative bare module specifiers in our tests (f.e. import ... from 'some-lib'), while code inside of src/ does not actually have any such library imports. If code in src/ did have library imports, I would have needed to add an importmap to my example at https://github.com/trusktr/prism-v2-test.

Screenshot 2023-07-05 at 5 35 39 PM

// "baseUrl": "./", /* Specify the base directory to resolve non-relative module names. */
// "paths": {}, /* Specify a set of entries that re-map imports to additional lookup locations. */
// "rootDirs": [], /* Allow multiple folders to be treated as one when resolving modules. */
// "typeRoots": [], /* Specify multiple folders that act like './node_modules/@types'. */
// "types": [], /* Specify type package names to be included without being referenced in a source file. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */
"allowUmdGlobalAccess": false, /* Allow accessing UMD globals from modules. */
Copy link
Author

Choose a reason for hiding this comment

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

Globals are typically considered bad. Disable type defs for globals by default, so that they don't sneak into our scope unintentionally from node_modules. Add them explicitly when/if needed.

// "moduleSuffixes": [], /* List of file name suffixes to search when resolving a module. */
// "resolveJsonModule": true, /* Enable importing .json files. */
// "noResolve": true, /* Disallow 'import's, 'require's or '<reference>'s from expanding the number of files TypeScript should add to a project. */

/* JavaScript Support */
"allowJs": true, /* Allow JavaScript files to be a part of your program. Use the 'checkJS' option to get errors from these files. */
"checkJs": true, /* Enable error reporting in type-checked JavaScript files. */
Copy link
Author

Choose a reason for hiding this comment

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

There's no JS, not needed.

// "emitDeclarationOnly": true, /* Only output d.ts files and not JavaScript files. */
// "sourceMap": true, /* Create source map files for emitted JavaScript files. */
"sourceMap": true, /* Create source map files for emitted JavaScript files. */
Copy link
Author

Choose a reason for hiding this comment

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

declaration plus declarationMap enables the feature where Go-To-Definition leads back to source code instead of declaration files. Really nice!

sourceMap enables source mapping for anyone who will further process the files. For example if they ever import from 'prismjs' and then compile their app with Webpack or Rollup, it is nice to be able to see errors/etc relative to the original source positions. If someone ever reports a bug, this will be handy because they can show the original location and source in the bug report, etc.

// "importHelpers": true, /* Allow importing helper functions from tslib once per project, instead of including them per-file. */
// "importsNotUsedAsValues": "remove", /* Specify emit/checking behavior for imports that are only used for types. */
// "importsNotUsedAsValues": "error", /* Specify emit/checking behavior for imports that are only used for types. */
"verbatimModuleSyntax": true,
Copy link
Author

Choose a reason for hiding this comment

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

importsNotUsedAsValues is deprecated. verbatimModuleSyntax is the new option. The rule is simple:

Any import with a type label gets removed. The rest is left behind. This is the least confusing compared to before: whatever imports you write are what you get (minus type imports). No room for error. It is now our responsibility to write or not write the imports we want, without TypeScript trying to guess.

Someone can use a tool like Webpack or Rollup to apply module tree shaking at their own risk of dealing with breakage, if they want to.

@@ -70,7 +66,7 @@

/* Interop Constraints */
// "isolatedModules": true, /* Ensure that each file can be safely transpiled without relying on other imports. */
// "allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */
"allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */
Copy link
Author

@trusktr trusktr Jul 6, 2023

Choose a reason for hiding this comment

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

Required for compatibility with Node ESM. In Node ESM, an imported CommonJS module's module object is always the default export inside of a Node ESM file, and that is what this enforces.

Prism code (in src/) does not import any libraries, let alone any CommonJS modules. However, it is possible that our infrastructure code (build, tests, benchmark) may import a library in CommonJS format, and because those files now run as Node ESM, this is required just in case those files do import a native Node CommonJS library.

@trusktr
Copy link
Author

trusktr commented Jul 6, 2023

Alrighty, I commented in all the relevant files to explain the changes. The rest of the vast majority of uncommented files are all just format updates (adding .js extensions, tweaking one or two imports for Node ESM compat, fixing __dirname).

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.

1 participant