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

Allow other module file extensions through config.importmap.accept #57

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mtgrosser
Copy link

Currently, importmaps require all locally pinned files to have a .js extension. This creates various problems when trying to pin assets which have their type (and thus file extension) transformed by the Rails asset pipeline:

  • Files with extensions other than .js are not picked up by pin_all_from
  • Changing files with other extensions than .js does not trigger a cache sweep.

This pull request proposes to make the accepted file extensions configurable through Rails.application.config.importmap.accept, which would default to ['js']. By adding other extensions, e.g. jsx or vue, importmaps will be able to pick up and map components which are transformed to ES modules by the asset pipeline.

Using gems like jass-react-jsx or jass-vue-sfc, native JSX and Vue Single File components can be integrated with Rails in a straightforward way, without the need for workarounds like htm or complicated build tools, by just placing them into app/javascript.

Copy link
Contributor

@radiantshaw radiantshaw left a comment

Choose a reason for hiding this comment

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

I went through the PR. For most of the part, it looks good. I was just wondering if it would be possible to not include the file extensions for the "imports" themselves. For example:

{
  "imports": {
    "components/HelloWorld": "/assets/components/HelloWorld.js"
  }
}

instead of:

{
  "imports": {
    "components/HelloWorld.vue": "/assets/components/HelloWorld.js"
  }
}

The thought process behind this would be that the Rails asset pipeline would convert the .vue files via Transformers, and the pin method would just be responsible for mapping them. The only problem I see with this approach is that it will not play out nicely with the pin_all_from method because it does touch the file system for knowing what files to pin.

To solve this, I've re-written the pin_all_method to behave kind of similar to the pin method in #72. If somehow we can merge these two ideas.

@mtgrosser
Copy link
Author

Thank you for going through the PR. The file extensions for non-JS files were included intentionally, in order to facilitate an easy transition from classic imports handled by bundlers like webpack or rollup to using import maps:

  • Moving to import maps only requires changing the imports from relative paths to abstract paths based on a common root, which also allows for easy switching back and forth between import maps and bundling, by having the bundler resolve all bare specifiers based on the common root. Most bundlers require the filename extension to determine how to handle non-JS imports.

  • It also allows using import maps for development and bundling for production, which we're successfully doing for a mid-sized project with hundreds of JS modules and Vue components.

  • Finally, the imports themselves provide more information about the kind of "special" module being imported when the file extensions are preserved.

The PR implementation pins all accepted extensions to .js, do you see any problem with this?

Regarding #72, I'll check out the PRs and see how the accept feature can be merged.

Regarding the "dev import maps" approach, it works as following:

We're determining which locally pinned assets to precompile by adding precompile as an optional keyword argument to the pin* methods and extending the MappedFile helper class, providing a precompile? method:

MappedFile = Struct.new(:name, :path, :preload, :precompile, keyword_init: true) do
  def precompile?
    not (path =~ ActionView::Helpers::AssetUrlHelper::URI_REGEXP || false == precompile)
  end
end

In development mode, the precompilable pins are provided by the import map and added to config.assets.precompile:

class ImportMap::Map
  def precompile
    expanded_packages_and_directories.values.select(&:precompile?).map(&:path)
  end
end

This way, we get fast reloads in development and can still have bundling for production.

@mtgrosser
Copy link
Author

There exists actually a problem with the current PR implementation and its retaining of the original extension:

Name derivation only works properly for files with one single extension. As soon as there is more than one, like in module.js.erb, the module name is no longer derived correctly.

This could be solved in two ways:

Either, by always removing all extensions for all module names as proposed above, or alternatively, by changing the way accepted extensions are defined: A file is considered accepted if one of its extensions matches the list of accepted extensions, i.e. in order to accept vue.erb, only vue would need to be accepted. Then remove all extensions up to and including.js or, if .js is not present, remove all extensions upto, and excluding the first accepted extension which is not .js. Which is fairly complicated, and might outweigh the benefits from keeping some extensions as set out above.

@mtgrosser mtgrosser force-pushed the config-accept branch 3 times, most recently from 5240f4a to ef7b00a Compare March 21, 2022 17:51
@mtgrosser
Copy link
Author

Updated PR to extension-agnostic pinning.

All accepted extensions will be mapped to .js while being stripped from the import specifier.
For example, jsx|vue|... files living under app/javascript/components can be pinned using

pin_all_from "app/javascript/components"

and imported by

import "Clock" from "components/Clock";

@mtgrosser mtgrosser force-pushed the config-accept branch 2 times, most recently from 19ca869 to e25a358 Compare March 21, 2022 20:28
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.

2 participants