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

Make Javascript imports consistent #1919

Open
simoncozens opened this issue Jan 6, 2025 · 19 comments
Open

Make Javascript imports consistent #1919

simoncozens opened this issue Jan 6, 2025 · 19 comments

Comments

@simoncozens
Copy link
Contributor

There are three kinds of Javascript import: relative (import { Foo } from "./foo.js", often used by people who bundle their imports into a single deliverable); root-relative (import { Foo } from "/foo.js", often used by people who deploy everything on the web) and the ES2020 absolute (import { foo } from "foo.js").

Currently Fontra uses an inconsistent mix of relative and root-relative:

Image

Inconsistency is confusing in itself, because it makes developers question why one thing is one way and another thing is another, but as you can see from the screenshot, this also confuses code analysis tools.

Root-relative imports confuse them because /core/var-glyph.js is interpreted as an absolute filesystem path, which is manifestly not true; but this can be worked around potentially by tweaking an import map. And relative imports confuse them similarly because the import is just not correct from a filesystem point of view: in this example, src/fontra/views/editor/edit-tools-pointer.js includes ../core/vector.js, which you would think means src/fontra/views/core/vector.js but is intended to mean src/fontra/core/vector.js! (And code analysis tools are helpful because, as you can see above, they're good at finding unused imports!)

I propose that core (and web-components) imports become ES2020 absolute imports:

import { VarPackedPath } from "core/var-path.js";
import * as vector from "core/vector.js";
import { EditBehaviorFactory } from "./edit-behavior.js";
import { BaseTool, shouldInitiateDrag } from "./edit-tools-base.js";
import { getPinPoint } from "./panel-transformation.js";
import {
  registerVisualizationLayerDefinition,
  strokeRoundNode,
   strokeSquareNode,
 } from "./visualization-layer-definitions.js";
import { translate } from "core/localization.js";
import { copyBackgroundImage, copyComponent } from "core/var-glyph.js";

I have this working locally and can make a PR if the idea is agreed.

@justvanrossum
Copy link
Collaborator

justvanrossum commented Jan 6, 2025

  1. This may break the cache busting logic, which I think currently expects import strings to start with "/" or "./". This could be fixed by tweaking the associated regex.
  2. The "views" are "plugins" registered in pyproject.toml, and can live anywhere. The import paths in them are only valid at runtime. I'm not surprised code analysis tools are getting confused. Perhaps an import map can help here?
  3. The imports under client/core are done so these imports also work with Node (for npm test). Do ES2020 absolute imports work in Node? It does look it could be a nice cleanup indeed.

More consistency is definitely desirable.

I want to move to an import-map solution, mostly because this will make it easier/possible for external plugins (hosted anywhere) to import stuff from Fontra (core/web-components). We could then manage the (JS) cache busting stuff by just manipulating the import map. (I don't think import maps work beyond JS source files, which is a bit of a bummer. Fontra's cache busting scheme works for most assets, not just JS.)

@simoncozens
Copy link
Contributor Author

The imports under client/core are done so these imports also work with Node (for npm test). Do ES2020 absolute imports work in Node? It does look it could be a nice cleanup indeed.

Not sure, my version of Node is too new for Fontra. ;-)

simon@Simons-iMac ~/others-repos/fontra *$ npm test

> test
> mocha test-js --reporter spec


 Exception during run: SyntaxError[ @/Users/simon/others-repos/fontra/test-js/test-classes.js ]: Unexpected identifier 'assert'

@justvanrossum
Copy link
Collaborator

Ugh, they made "assert" a reserved word, just like that? Or is ES on board with this?

@simoncozens
Copy link
Contributor Author

That's not the problem, the problem is that the import assertions syntax - which was an ES proposal - has developed into a more generic "import attributes" syntax. It's still a proposal (stage 4) but people are using the new syntax.

@justvanrossum
Copy link
Collaborator

Ahh, then it's this:

import classesSchema from "../src/fontra/client/core/classes.json" assert { type: "json" };

It needs to be replaced with something more contemporary then.

@simoncozens
Copy link
Contributor Author

simoncozens commented Jan 13, 2025

Unfortunately even if we fix the import assertions syntax and the tests work fine in Node, we can't use absolute path imports in the browser if we're not using something like rollup/webpack to resolve those imports statically. So this idea may be a non-starter. Wait, I think I may be able to importmap this...

@simoncozens
Copy link
Contributor Author

No, this is a nightmare. Having the browser resolve the imports with an importmap means they don't go through the cache-buster. In some cases (e.g. inline-svg) things fail because the import is loaded twice, once through the cache-buster and once directly. Resolving statically and going through a bundler is the only way it's going to work, but that's a way more invasive change.

@justvanrossum
Copy link
Collaborator

We can replace the (JS) cache busting with a cache busting import map. I have indeed found in the past that using an import map does not work well with the (current) cache buster.

While I'd love to improve support for bundlers, I don't want to depend on one.

Some of the things to do I posted here should make things easier. Esp. to make the "views" be part of the proper assets tree on disk, instead relying on server juggling. Then we could switch to all relative imports.

@simoncozens
Copy link
Contributor Author

Esp. to make the "views" be part of the proper assets tree on disk, instead relying on server juggling.

Ah, I presumed that the idea of views being pluggable (and defined in the pyproject.toml) is to allow people to add their own out-of-tree views directories which then get overlaid on top of the server web root. If we're content to just serve a static assets directory then the architecture gets much simpler (you don't even need to "register" views as entry points, they're just more files to serve) ... but we lose that pluggability.

@justvanrossum
Copy link
Collaborator

The idea is that the builtin views become integrated (and indeed don't need to be registered), but that views are still pluggable with the existing mechanism. So any plugins made that way would be more tightly coupled to the server implementation.

@simoncozens
Copy link
Contributor Author

So it looks like there are a lot of constraints we want to fulfil here:

  • Allow for deployment with a bundler and, separately, allow deployment in another directory. This means we can't use root-relative paths - node doesn't like them and bundlers don't support them, and on the web, they'd be incorrect if served from a subdirectory.
  • Allow for deployment without a JS bundler, pass imports through Fontra server's home-brew cache buster. This means we can't use absolute imports - normally you'd have a bundler resolve them, but without that, you have to get the browser to resolve them, and that comes up against the cache buster.
  • Support layering on external out-of-tree views and have them resolve correctly. And this means we can't use relative paths either, because they won't be correct.

I feel like some of these constraints fight against the standard ways of doing things in JS web apps, but you're entitled to make those architectural decisions. The upshot, though, is that I just don't see a solution which fulfils all of these.

@justvanrossum
Copy link
Collaborator

The first two should be able to co-exist when using all relative paths.

There third (layering on external out-of-tree views) seems the most problematic. I'd think these could still use absolute imports (and then depend on a more restircted type of deplyment). But since we don't have an actual example of such a plugin (ignoring the current built-in views), perhaps I should for now let go of this idea.

@simoncozens
Copy link
Contributor Author

simoncozens commented Jan 14, 2025

I think if you're prepared to rethink one of your constraints, I'd highly, highly recommend you to rethink the opposition to requiring a bundler. It is just the way that JS applications are developed these days, and for good reason; it would have various advantages for Fontra:

  • You get static import resolution, which catches errors more quickly.
  • You get cache busting for free.
  • You get the ability to use WASM and potentially TS in the future.
  • You get cleaner handling of third-party modules, dependency management, etc. meaning you have to reinvent fewer wheels.
  • You get fewer HTTP requests and a faster client experience.
  • The server code gets simplified; views become a Javascript module containing some JS and some assets, which inject themselves into the bundler config.
  • You get better modularity. (For example, the "core" can become an independent @fontra/core JS module which is independently tested and declares its own interface.)
  • It's just a more standard way of development, making it more comfortable for JS programmers to work on.

I don't quite understand the reasons for your opposition to requiring a bundler - but I do note that in practice, you require a bundler already for third-party. So why not embrace it and get the benefits above?

I'm prepared to put together a branch to demonstrate this if it would help you evaluate it.

@justvanrossum
Copy link
Collaborator

I'm not opposed to maybe requiring a bundler for deplyment. But while developing, I tweak some CSS or JS, do a reload in the browser and I see the result. This is incredibly convenient while developing, and I'd hate to lose that instant dev cycle.

@simoncozens
Copy link
Contributor Author

While working on FontraWeb, I have

$ npx webpack build --watch --config webpack.config.cjs

running in another terminal, and I get that instant dev cycle. We can wrap that up as npm run dev or whatever.

@justvanrossum
Copy link
Collaborator

How long does the build take?

@simoncozens
Copy link
Contributor Author

webpack 5.97.1 compiled successfully in 721 ms

@justvanrossum
Copy link
Collaborator

Not bad.

Yet, it adds complexity to my dev experience, and when we fix the import inconsistencies and the out-of-tree views, I really don't see why we can't have both.

@simoncozens
Copy link
Contributor Author

Ah well.

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

No branches or pull requests

2 participants