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

Use dynamic import to load plugin client code #320

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Use dynamic import to load plugin client code #320

merged 1 commit into from
Jul 15, 2024

Conversation

nrn
Copy link
Member

@nrn nrn commented Jun 18, 2024

I know it seems insane, but this seems to just work.

By using the built in import expression we can dynamically load scrips like we have been, but those scrips in turn can now use import statements and easily load their own dependencies without a build step. For a plugin that would often mean pulling from its own plugin folder like

import { test } from '/plugins/mech/test.mjs'

The grunt change is required to prevent babel from replacing native dynamic imports with it's own stuff. This is the only call to loadScript so it was easy to cut the unused options argument, and convert the response to normal promise methods.

This is going to require a lot of testing.

@WardCunningham
Copy link
Member

WardCunningham commented Jun 19, 2024

I wonder how far back in browser versions we would have to go before this won't work? On a truly obsolete browser one would still have access to the built-in plugins. This would be a feature simplifying the life of new plugin authors and certainly a step in the right direction.

Oh, I see this has been considered in #189

@paul90
Copy link
Member

paul90 commented Jun 19, 2024

A number of plugins already import their own dependencies, without a build script. Albeit using a dynamic import, like const Plot = await import('https://cdn.jsdelivr.net/npm/@observablehq/plot@0.6/+esm').

But switching to static import would simplify importing things that are always needed, but dynamic is probably probably preferred for things that are not always needed.

Should work in quite old browsers, https://caniuse.com/?search=import(),

@nrn
Copy link
Member Author

nrn commented Jun 19, 2024

Yeah, that's a good point, we could use dynamic imports in plugin code since they can be used from either scripts or modules in the browser, or in node from CJS or .jsm, and can cross load any of them from any situation. This just seems like a really convenient boundary to put dynamic imports at so we can use import statements to simplify things in plugin land.

@paul90
Copy link
Member

paul90 commented Jun 20, 2024

Some plugins, such as code, which themselves are using dynamic plugins fail with this change. Others appear to work.

@nrn
Copy link
Member Author

nrn commented Jun 20, 2024

Aargh, caught out by strict mode compatibility! It's an easy fix in the code plugin, but I'm not sure how many might be a problem.

@paul90
Copy link
Member

paul90 commented Jul 1, 2024

Have created PRs to update the D3 plugins.

@nrn
Copy link
Member Author

nrn commented Jul 3, 2024

Took a look over them and these all look good, thank you!

I'll try and get them installed tonight or tomorrow to take them for a quick test drive.

@paul90
Copy link
Member

paul90 commented Jul 14, 2024

Finally got around to a rc for the change to the code plugin - it is installed on the test server. The only update not installed there is the change to the graphviz plugin.

@paul90 paul90 merged commit ed5d545 into master Jul 15, 2024
3 checks passed
@paul90
Copy link
Member

paul90 commented Jul 15, 2024

Merging as discussed in Sunday's call.

Have published a release candidate, but if used will need the updates from the plugins referenced above.

The next release will also include the changes in #321.

@paul90 paul90 deleted the nrn/import branch July 15, 2024 08:55
nrn added a commit that referenced this pull request Jul 30, 2024
paul90 pushed a commit that referenced this pull request Aug 3, 2024
paul90 added a commit that referenced this pull request Aug 5, 2024
* nrn/bind-error:
  0.30.0-rc.1
  update package lock
  Pull in import from #320
  Update lib/plugin.js
  Asyncify plugin.js
  decaffeinate lib/plugin
  renamed:    plugin.coffee -> plugin.js

# Conflicts:
#	lib/plugin.coffee
#	package-lock.json
#	package.json
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.

3 participants