kentcdodds / babel-plugin-macros

🎣 Allows you to build simple compile-time libraries
https://npm.im/babel-plugin-macros
MIT License
2.62k stars 135 forks source link

Macro import paths with Windows separators are ignored #125

Closed haltcase closed 5 years ago

haltcase commented 5 years ago

Relevant code or config

import { _ } from '.\\macro'
import { it } from 'E:\\dev\\param.macro\\macro'

I came across this when testing param.macro and have noticed that tests will fail on Windows — the import path I generate for testing (an absolute Windows style path) is ignored by babel-plugin-macros but is also further incompatible with it.

What you did:

Compile

What happened:

The macro is not activated.

Reproduction repository:

  1. git clone https://github.com/citycide/bpm-repro.git
  2. yarn
  3. yarn build
  4. dist.js is created but was not transformed

Problem description:

  1. the macro regex only accounts for a unix path separator (/)
  2. even once 1) is fixed, the resolve module throws MODULE_NOT_FOUND

Suggested solution:

First the regex would need to be updated to something like this:

https://github.com/kentcdodds/babel-plugin-macros/blob/2d57c60adadd8904db9624614faffc34a251c40d/src/index.js#L5

- const macrosRegex = /[./]macro(\.js)?$/
+ const macrosRegex = /[./\\]macro(\.js)?$/

However this isn't enough as once applyMacros is actually called, the resolve module will throw:

Error: Cannot find module '.\macro' from 'E:\dev\param.macro'

There are existing issues that mention Windows but they all relate to tests.

kentcdodds commented 5 years ago

Hi @citycide,

I didn't realize that you could do a path like that. Is that permissible according to the ES spec?

haltcase commented 5 years ago

It's possible that it's not allowed in import sources and that it has to be a valid URI. node's require on the other hand is simply given a path and a Windows path is perfectly valid.

I could generate a require instead but this would still not work, and then we need to decide if we're going to separate the source handling logic to follow the differing specs of import and require.

sompylasar commented 5 years ago

the import path I generate for testing (an absolute Windows style path)

Some people are genuinely curious why you need Windows paths.

https://twitter.com/rossipedia/status/1148984649735000064

rossipedia commented 5 years ago

I just realized that I'm assuming that absolute Windows paths are not supported by the ES spec, but I could be completely wrong about that. Would replacing the \\ separator with / work?

i.e. change 'E:\\dev\\param.macro\\macro' to 'E:/dev/param.macro/macro'

rossipedia commented 5 years ago

So, I'm not sure if they're supported by the ES spec, but it looks as though they're not supported by Node:

image

kentcdodds commented 5 years ago

Alright. I'll go ahead and close this one then 👍 Thanks!

resynth1943 commented 5 years ago

Sorry for asking here, @rossipedia, but what is that font?

haltcase commented 5 years ago

@sompylasar I don't necessarily. I was just using Node's path.join and process.cwd to generate the path during testing, and on Windows those paths make everything broken. I guess I can unixify everything but that went against my cross-platform instincts. :laughing:

@rossipedia that's odd because I was able to run a very similar example and a Windows path worked just fine.

sompylasar commented 5 years ago

I believe Node.js source can be considered a reference implementation.

It says that the thing you put in the import statement is called "module_request" (which not necessarily maps to a file system path 1:1). https://github.com/nodejs/node/blob/2983eaca84e9e79c19dba09fb036fe55131b8a5d/deps/v8/src/ast/modules.h#L37

It goes to GetModuleNamespace which is an abstract ECMAScript operation https://github.com/nodejs/node/blob/8aca934009225b30bff7d7927d2eb9f667dbff9f/deps/v8/src/runtime/runtime-module.cc#L38

https://tc39.es/ecma262/#sec-getmodulenamespace

That mentions the HostResolveImportedModule abstract operation which depends on the implementation of the host environment.

So the answer is actually "the spec says it depends on the host environment".

kentcdodds commented 5 years ago

the spec says it depends on the host environment

😬 I didn't vote for that, lol

sokra commented 5 years ago

If the host environment is webpack, absolute windows paths are allowed. Relative paths with windows separator are not.

kentcdodds commented 5 years ago

I personally don't think it's worthwhile supporting this, but because it's so easy to do so (update the regex) I'm actually fine if someone wants to make a PR for it.

rossipedia commented 5 years ago

@resynth1943 no problem! it's Victor Mono