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

Source handler doesn't run for a file extension if it is a file name #3985

Closed
stevezhu opened this issue Mar 21, 2015 · 11 comments
Closed

Source handler doesn't run for a file extension if it is a file name #3985

stevezhu opened this issue Mar 21, 2015 · 11 comments

Comments

@stevezhu
Copy link

@stevezhu stevezhu commented Mar 21, 2015

When using a file name as a file extension when registering a source handler as cited in the Plugin.registerSourceHandler docs

Plugin.registerSourceHandler('some_json.json', function(compileStep) {
  console.log('In some_json.json source handler');
});

"In some_json.json source handler" isn't logged unless you also add a source handler with just json

Plugin.registerSourceHandler('json', function(compileStep) {
  console.log('In json source handler');
});

Note: this works fine for api.addFiles in packages

reproduction: https://github.com/stevezhu/meteor-source-handler-bug
using osx mavericks 10.9.5 if it's relevant

@glasser
Copy link
Member

@glasser glasser commented Mar 26, 2015

So, you're right that there's a bug here, but the bug is that the exact filename match ever works at all. The feature is documented as:

The file extension that this plugin should handle, without the first dot. Examples: "coffee", "coffee.md".

It's pretty explicit that this is supposed to be an extension, and most of the code (eg, the code that looks for relevant files inside an app) correctly handles this. The fact that, in certain odd cases, the full filename can match, is a bug that I'll fix. Thanks!

@avital
Copy link
Contributor

@avital avital commented Mar 26, 2015

I think some packages may have depended on this behavior. (Unfortunately I don't recall specific examples)

@glasser glasser closed this in d226fd1 Mar 26, 2015
@glasser
Copy link
Member

@glasser glasser commented Mar 26, 2015

@avital Would love to know. I checked https://atmospherejs.com/mizzao/build-fetcher and it uses extension.

And note that the behavior worked inconsistently here: it would work in a package, or in an app if there was a suffix of the extension also declared (as @stevezhu showed). Because the code that looks in your app for files to process does assume that extensions are extensions:

        var sourceInclude = _.map(
          extensions,
          function (isTemplate, ext) {
            return new RegExp('\\.' + utils.quotemeta(ext) + '$');
          }
        );
@stevezhu
Copy link
Author

@stevezhu stevezhu commented Mar 26, 2015

Ahh I see, thanks for that clarification and glad I unintentionally uncovered another bug.

@johanholmerin
Copy link

@johanholmerin johanholmerin commented Apr 11, 2015

mquandalle:bower and numtel:publicsources both rely on this.

numtel added a commit to numtel/meteor-publicsources that referenced this issue Apr 11, 2015
* Handles this bug when used in combination with mquandalle:bower
  meteor/meteor#3985
@glasser
Copy link
Member

@glasser glasser commented Apr 15, 2015

@mquandalle @numtel You may way to try to address this before the next Meteor release.

(I'm thinking of adding filename: support to a build plugin refactoring though.)

@johanholmerin
Copy link

@johanholmerin johanholmerin commented Apr 15, 2015

@Differential's Vulcanize also uses this pattern.

@numtel
Copy link

@numtel numtel commented Apr 15, 2015

@glasser Already taken care of. My packages are no longer dependent on this bug.

@avital
Copy link
Contributor

@avital avital commented May 26, 2015

https://github.com/elidoran/cosmos-browserify also depends on this, or at least the example in the README (cc @elidoran)

@elidoran
Copy link

@elidoran elidoran commented May 26, 2015

Good to know, thank you @avital
I'll revise cosmos:browserify accordingly.

EDIT: revised and published. thank you.

glasser added a commit that referenced this issue Jul 9, 2015
This is in addition to registering for extensions. Note that only the
new SourceProcessor APIs allow this, not registerSourceHandler.

The filename in question is the basename of the file.  The file can be
found in any directory in any package.  If you want to be more picky,
you can just ignore other ones in your processFilesForTarget.

This introduces the SourceProcessorSet abstraction, which simplifies a
lot of repeated code around matching filenames with processors and
avoiding duplicates.

Missing tests.

See also #3985.
@glasser
Copy link
Member

@glasser glasser commented Jul 13, 2015

Note that a forthcoming version of Meteor (implemented on the batch-plugins branch) allows plugins using the new registerCompiler API to register for specific filenames in addition to extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.