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

Do not treat client and server directories specially in packages. #10414

Merged
merged 3 commits into from Jan 11, 2019

Conversation

Projects
None yet
6 participants
@benjamn
Copy link
Member

commented Jan 11, 2019

The client/ and server/ directories should be meaningful only for organizing application code.

Before this commit, if a Meteor package contained a client/ or server/ directory, any modules inside it could be imported, but they would not be properly compiled, because the PackageSource#_findSources method would ignore them in its initial scan, which determines the set of modules that get passed into compiler plugins.

Fixes #10393.

@benjamn benjamn added this to the Release 1.8.1 milestone Jan 11, 2019

@benjamn benjamn self-assigned this Jan 11, 2019

benjamn added a commit that referenced this pull request Jan 11, 2019

benjamn added a commit that referenced this pull request Jan 11, 2019

Bump compiler.BUILT_BY and LINKER_CACHE_SALT.
PR #10414 changes the behavior of the build system in a subtle way that
does not automatically trigger recompilation.

@benjamn benjamn changed the base branch from release-1.8.1 to devel Jan 11, 2019

benjamn added some commits Jan 11, 2019

Bump compiler.BUILT_BY and LINKER_CACHE_SALT.
PR #10414 changes the behavior of the build system in a subtle way that
does not automatically trigger recompilation.

@benjamn benjamn force-pushed the special-client-server-dirs-in-app-only branch from 124dfad to 268692a Jan 11, 2019

@benjamn benjamn merged commit 20da99c into devel Jan 11, 2019

2 of 6 checks passed

ci/circleci: Get Ready CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
@nabiltntn

This comment has been minimized.

Copy link

commented Jan 14, 2019

Just to tell that i tried the build with the current version of devel and it works fine ! Thanks

@aliogaili

This comment has been minimized.

Copy link

commented Feb 20, 2019

We have a different architecture and we build our code inside the packages.

We've been depending on the client folders within packages to speed things up, but after this merge I don't think this would be feasible. I think the consistency of the dev experience inside/outside the packages was a valid assumption.

@manueltimita

This comment has been minimized.

Copy link

commented Apr 4, 2019

I've been trying to understand from the issue which prompted this patch: is removing this special treatment of client and server the only possibility of fixing it? If Meteor really wants to drop the packages completely, then this move would be understandable. Otherwise, it precludes one of the really great features of Meteor, the possibility of developing truly modular applications, in the vein of Vulcan for instance.

It is a real shame that we don't have this anymore, as it makes it more difficult to develop large yet pluggable applications.

@eric-burel

This comment has been minimized.

Copy link

commented Apr 9, 2019

Can you explain more clearly the technical consequences of this? What happens when I import my-package server-side for example, and client-side? As-is, this update is worrying for application relying massively on Meteor isomorphism and package system, Vulcan is a good example.

Edit: I think this thread answers my question: https://forums.meteor.com/t/meteor-1-8-1-client-and-server-directories-in-packages/48452/3

@benjamn

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

The right way to fix this will be for Meteor to respect the "where" argument to api.addFiles(..., "client") and api.mainModule(..., "server") in package.js when deciding whether a file change is client-only or requires a server restart. Using client/ and server/ directories in packages was an accident that caused bugs like #10393.

Note: the current implementation of client refreshing needs to be improved to use the "where" argument correctly, so this comment is a statement of intention, not immediately useful advice. Sorry for the inconvenience, in the meantime!

@benjamn benjamn deleted the special-client-server-dirs-in-app-only branch Apr 9, 2019

@manueltimita

This comment has been minimized.

Copy link

commented Apr 9, 2019

Thank you for checking this up, @benjamn. We are already using the where argument, exactly as you describe (actually, since Meteor 1.3, so we never had to rely on the 'special treatment' ever again). But the problem with the full rebuild when client files are changed is still there nonetheless, and it can be quickly replicated as described in #10449.

It seems like this is an older, hidden bug, as described there:

(As a side note, also in 1.8.0.2 a server rebuild is done, when client js is located in packages/custom/client.js so the original problem is much older i suppose)

@benjamn

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@manueltimita Right, what you're doing should be enough to make this work without any code changes on your side. However, there are some flaws in the way Meteor currently determines whether it's safe to do a client-only refresh (namely, it relies on client/ directories, not the actual dependency graph). Fixing those flaws would be beneficial for everyone who uses Meteor. I've updated my previous comment to make this somewhat clearer.

@manueltimita

This comment has been minimized.

Copy link

commented Apr 9, 2019

Thank you for the clarification, @benjamin.

@aadamsx

This comment has been minimized.

Copy link

commented Apr 12, 2019

I'm confused. I'm currently on Meteor 1.8 and have the "special" directories server/client/both. This to me makes sense and I don't know why anyone would want to change that.

What I don't like is the imports directory. I think things should act like they do today inside the imports directory, where server/client/both mean what they say, but without the imports directory.

Are we still going to have special directories server/client/both in the future?

@benjamn

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@aadamsx We're talking about packages here, not application code. We're going to keep the client/ and server/ directories for application code for the foreseeable future, though we might be able to do a better job detecting client-only refreshes using the dependency graph, which would effectively render the client/ directory unimportant.

If you would like all your application code to be loaded lazily by default, so you don't have to worry as much about the imports/ directory, you can use the meteor.mainModule configuration option in package.json that was introduced in Meteor 1.7:

  • Applications may now specify client and server entry point modules in a
    newly-supported "meteor" section of package.json:
    "meteor": {
      "mainModule": {
        "client": "client/main.js",
        "server": "server/main.js"
      }
    }
    When specified, these entry points override Meteor's default module
    loading semantics, rendering imports directories unnecessary. If
    mainModule is left unspecified for either client or server, the
    default rules will apply for that architecture, as before. To disable
    eager loading of modules on a given architecture, simply provide a
    mainModule value of false:
    "meteor": {
      "mainModule": {
        "client": false,
        "server": "server/main.js"
      }
    }
    Feature #135
    PR #9690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.