facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.76k stars 26.87k forks source link

Document src/node_modules as official solution for absolute imports #1065

Closed gaearon closed 6 years ago

gaearon commented 7 years ago

After a long discussion in #741 it seems to me that src/node_modules seems like the best solution for absolute imports.

It has the following benefits:

This issue is a call to help to make this approach the official one. We need to:

If you want to work on either please leave a comment and then submit a PR! Cheers.

fadeojo commented 7 years ago

I want to help but I will need someone to shadow.

gaearon commented 7 years ago

Feel free to jump on either of those issues and let me know if you need any help!

tbranyen commented 7 years ago

This is a very interesting idea! I'm wondering though if it'd make more sense to support a top-level packages directory (exactly like lerna) and symlink that to src/node_modules. This binding could exist for all create-react-apps.

src/
src/node_modules -> ../packages
packages/
public/

The reasoning for this is that typically your external modules are general purpose and not application-specific (therefore not desirable in the application source directory), and to support something like lerna. Maybe you want to do code splitting via npm packages? This would allow you to support lerna and create-react-app in the same project.

For selfish reasons, this would allow me to use create-react-app to kitchen sink/playground a bunch of lerna components.

viankakrisna commented 7 years ago

what's the issue with cross-env NODE_PATH=./src react-scripts start ? I think it's cleaner and more clear rather than having a node_modules inside src folder.

gaearon commented 7 years ago

Most people don't know about cross-env so they just write commands that don't work on Windows. This hurts open-source examples (e.g. people can't run a React example because of its reliance on Bash environment variable syntax). NODE_PATH is also considered legacy and slightly discouraged so it doesn’t feel right to promote this as a solution. Just using src as the root can also be a bit surprising, and the explicitness of node_modules has some benefits (e.g. you know for sure they’ll have priority over regular node_modules).

Ultimately I just want both ways to work, and we can document them. People can then choose whichever way they prefer.

gaearon commented 7 years ago

support a top-level packages directory (exactly like lerna) and symlink that to src/node_modules

I believe this is what I suggested in the opening comment (but only as opt-in):

If you really want to, you can run ln -s src/node_modules src/packages and use the "nicer" packages (or any other) folder.

It has to be opt-in though, please read the discussion about Windows issues in #741.

daltones commented 7 years ago

I personally like the idea of a src/node_modules, just because it's absolutely free of hacks.

But right now there's some problems with linters:

kompot commented 7 years ago

Another way of managing absolute paths that might have come up but could not find it in https://github.com/facebookincubator/create-react-app/issues/741

We've been using wix's wml https://github.com/wix/wml to get any required project structure with:

As project's readme states — they just use watchman to watch for certain dirs and copy files to node_modules subfolders. We've added some additional scripts to automate bootstrapping but it turned out to work really smooth.

Sure it's not an ideal solution but it seems to work really nice and does not force you to follow any conventions so it's easily can be applied to any project.

What do you think?

AdamTyler commented 7 years ago

I've created a link src/node_modules to src/common and this allows me to import a component using import Rating from 'components/Rating' (components is a folder in common).

However, when I run my tests, jest gives me the following error:

Test suite failed to run

    SyntaxError: Unexpected reserved word

on the line where I am doing that import.

Is there something I need to add to my jest config to make this work properly?

gaearon commented 7 years ago

@AdamTyler

Please see the first post in this thread 😉

screen shot 2016-12-01 at 17 49 42
adiachenko commented 7 years ago

Hey, guys, just wanted to leave a note here because there seem to be some confusion in the community regarding Windows 10 Creators Update (coming in a few weeks) that will supposedly allow for better symlink support.

Some interesting facts:

In short, for all intents and purposes you can still assume that Windows doesn't have proper symlink support so please don't plan any features that rely on them unless they're opt-in.

alex35mil commented 7 years ago

In my mental model node_modules is something I can easily rm -rf and regenerate w/o worries to lose anything. I never expect any app code in there. A lot of various configs ignore it for the same reason: in most of the cases, it's something not related to user's code. Also, usually you have to move a whole app in this directory.

Guria commented 7 years ago

@alexfedoseev to avoid this mental confusion suggestion implies putting it under src folder. On the other hand, you could freely remove any files under source control and regenerate them w/o worries using git checkout HEAD

adiachenko commented 7 years ago

@alexfedoseev Yeah, it has been said already that this is introduced not because people like it, but because there isn't a better option. The only close enough cross-platform alternative is resolve.modules and it blows because all code intelligence facilities end up being broken (that and the fact that it relies on webpack).

Rather than absolute imports it's a solution for shared modules (see example). Obviously, you won't be moving an entire app under node_modules folder, so the current name for this is somewhat misleading.

Janpot commented 7 years ago

will there also be a way to build and npm publish these as separate components?

azz commented 7 years ago

@Janpot I'd look at a tool like lerna if you're publishing multiple packages from one repo.

Janpot commented 7 years ago

@azz That's not really the reason why I ask this question. I mean adding what would be the component equivalent of create-react-app. "How to create a reusable react component with no build configuration?". The "standard way" let's say, to create and distribute components. When I look around the landscape, some components are ES2015 bundles, some have external CSS, some have styles bundled with a style loader, some create UMD bundles and duplicate all my dependencies,... Each have their own flavour of build configuration. It's annoying and counter-productive to figure out this toolchain each time you try to contribute to a repo. a solution like create-react-app that is focused on components could be very helpful here. It might not cover 100% of the use cases, but neither does create-react-app.

viankakrisna commented 7 years ago

@Janpot nwb?https://github.com/insin/nwb/blob/master/docs/guides/ReactComponents.md#developing-react-components-and-libraries-with-nwb

Janpot commented 7 years ago

@viankakrisna Indeed, sort of the create-react-app equivalent of that nwb feature I guess.

sidoshi commented 7 years ago

If I understand correctly, the only reason we are not using NODE_PATH env by default is because it would conflict with node_modules. But it's much cleaner than adding node_modules in src and configuring jest to ignore node_modulesinside src.Instead, we could use NODE_PATH=src by default with cross environment support and document it properly that the modules in src have preference over node_modules.

Further, we could enforce the direct folders or files inside src be in PascalCase and as npm doesn't allow package names to start with capital letters,

import React from 'React'

won't conflict with the react in node_modules and the user will know that it is an absolute import from src.

tbranyen commented 7 years ago

@doshisid Wouldn't that only work in case sensitive filesystems? If so, :thumbsdown:

sidoshi commented 7 years ago

We could find a common case for all filesystems and enforce that. Like maybe prefixing with _

shendepu commented 7 years ago

For the absolute import, beside of packages approach which is good pattern to split code into packages, why not simply put the src as the first one for webpack resolve.modules?

In the fractal project structure

src
-- components
-- routes
    -- jobs
      -- components
      -- actions
      -- list
        -- components
        -- actions
      -- detail
        -- components
        -- actions

for code in src/routes/jobs/details/actions to import something from src/routes/jobs/actions, it still needs to use ../../actions to relative import. This is simple case, with nest structure getting deeper, it will be something like ../../../../actions which is hard to read what is imported. With src as root resolved, it is crystal to say import from src/routes/jobs/actions

And with packages approach, even routes is package under packages folder, this long back path relative import is still inevitable.

=============== UDATE: =============== Find it is simple to add NODE_PATH=src in .env file so that webpack modules.resolve will have src. even better to add NODE_PATH=src:packages, then no npm link need for packages in packages folder

Danilo-Araujo-Silva commented 7 years ago

For me using node_modules inside src is not beeing a option because jest is crashing. Very annoying.

So my solution was using NODE_PATH=src in my package.json and in WebStorm/Intellij I simply did:

Right click on src folder -> Mark Directory as -> Sources Root

Voi lá! Everything is working fine! (go to files, open classes, moving files to another directory automatically change the references)

A note about NODE_PATH is that in the current docs (Node 8.1.2) is not saying that NODE_PATH is legacy or will be deprecated but that it is not used as it was used before:

NODE_PATH is still supported, but is less necessary now that the Node.js ecosystem has settled on a convention for locating dependent modules. Sometimes deployments that rely on NODE_PATH show surprising behavior when people are unaware that NODE_PATH must be set. Sometimes a module's dependencies change, causing a different version (or even a different module) to be loaded as the NODE_PATH is searched.

nazreen commented 7 years ago

the docs quote above convinces me that it's not deprecated, contrary to what OP has implied. I'm a 'new' developer myself and I feel like this node_modules solution will be confusing or seem strange to some. the alternative of using NODE_PATH seems like the better way.

cecilemuller commented 7 years ago

NODE_PATH require an extra dependency to be cross-platform, tools has to specify the extra variable, and it works here only because it's the toplevel (as the react app isn't a module that can be imported in another module), otherwise it would introduce extra complexity there too.

But pick whatever you prefer: I'm not fond of /src/node_modules either, just is the most pragmatic in the end given the way Node is at the moment.

bmac commented 7 years ago

I tried playing around with using this approach in my project and I noticed react-scripts start wasn't picking up changes inside src/node_modules because it was ignoring node_modules. I opened https://github.com/facebookincubator/create-react-app/pull/2760 to update the dev server config to play nicely with src/node_modules.

icopp commented 7 years ago

For absolute imports, it seems like it'd be a lot better to have some kind of special prefix and set up webpack's alias functionality for it. For example, my personal projects use:

{
  resolve: {
    alias: {
      "@": path.resolve(__dirname, 'src')
    }
  }
}

...after which you can reference files via...

import App from '@/App' // the file is at `src/App/index.jsx`

You can also inject the same schema into Jest:

"moduleNameMapper": {
  "^@/(.*)$": "<rootDir>/src/$1"
}

It keeps a clear distinction from npm-tracked node_modules, avoiding the "I can't get it to work and I don't understand why" potential of having overlapping names or having imports start with just ./.

gaearon commented 6 years ago

Closing in favor of https://github.com/facebookincubator/create-react-app/issues/1333. src/node_modules is too weird to most people and not very ergonomic.

There's been a few underlying issues with Jest which have been fixed so we can take another shot at this.

bjankord commented 6 years ago

Sorry to resurrect an old issue, though with #1333 being not quite ready, is src/node_modules a viable option for people to wanting aboslute imports @gaearon? I like the idea that it relies on Node resolution mechanism, but wasn't sure if this approach was discouraged.