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

Make resolution cross-env in filesystem plugin and base package #1128

Conversation

@SleeplessByte
Copy link
Contributor

commented Apr 15, 2019

  • Makes sure it's relative to the location root, not site root
  • Makes sure it's resolving paths, indepedent of file system
  • Makes sure it starts with a slash in case there is no directory structure

Description

Changes the resolution in react-static-plugin-source-filesystem to be cross-env compatible.

  • Stop using hardcoded path separators. These are not the same across environments.
  • Use path.relative for relative resolution instead of replace. This normalises paths first, so it works across environments.
  • Cutoff the extension using path.basename and path.extname, which works across environments.
  • Prefix with a slash if necessary (top-level pages)
  • Change the comment to be correct: make relative to location root, not site root.

This works with path.resolve(...) and ./src/pages.

Changes the resolution in react-static to be cross-env compatible.

  • Throw an error if a plugin resolves a windows path (it expects POSIX paths by then)
  • Use join whenever dealing with real paths
  • Add tests for the behaviour above

Additionally, changes the webpack.config.prod.js to use cross-env resolution for the paths that are considered external.

Changes/Tasks

  • Changed code

Motivation and Context

Fixes #1094
Fixes #1122
Fixes #1123

Screenshots (if appropriate):

Types of changes

  • Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have updated the documentation accordingly
  • I have updated the CHANGELOG with a summary of my changes
  • My changes have tests around them
Make resolution cross-env
* Stop using hardcoded path separators. These are not the same across environments.
* Use path.relative for relative resolution instead of replace. This normalizes paths first
* Cutoff the extension using path.basename and path.extname, which works across environments
* Prefix with a slash if necessary
@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

I've not added tests yet, and not thoroughly tested this in general.

  • This works in my project.
  • Opened the PR as tracking PR and accepting comments / ideas before tests are done.

@SleeplessByte SleeplessByte changed the title Make resolution cross-env react-static-plugin-source-filesystem: Make resolution cross-env Apr 15, 2019

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

This looks great to me. Let me know when you're ready to merge. I am!

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@tannerlinsley I just want to add a few tests to cover this in perpetuity.

Currently making sure this all works with nested paths et cetera, adding tests:

  • both types of directory separators
  • using .resolve in location (absolute, using system separators)
  • using .join in location (relative, but correct)
  • using hardcoded / in location (relative, and incorrect)
  • nested paths (both \ and /)
@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

Awesome!

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

FWIW: I tested out nested paths etc; these all work as intended.

yarn build still fails, probably similar issue and not related to this plugin :) (see #1123)

Failed exporting HTML for URL blog/post/1 (../src/containers/Post):
Cannot convert undefined or null to object

  • Function.keys

  • Routes.js:38 keys
    [site-name]/[react-static]/src/browser/components/Routes.js:38:1
    4

SleeplessByte added some commits Apr 16, 2019

Remove linebreak-style rule
This is covered by git and other svn; it will automatically correct the linebreak style.

https://eslint.org/docs/rules/linebreak-style#using-this-rule-with-version-control-systems

> For example, the default behavior of git on Windows systems is to convert LF linebreaks to CRLF when checking out files, but to store the linebreaks as LF when committing a change

This rule is therefore redundant.
Add cross-env
This correctly passes environment variables to the process across environments.
Fix non-bourne shell quotes
Single quotes won't work across shells. In this case the argument never needs quotes.
@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

⚠️ Don't merge yet ⚠️

Does not yet solve the build issue on windows.

The issue lies in _useStaticInfo.useStaticInfo. It returns an empty hash {}.

}
// Instead of using path.sep, we always want to test for all of them. This makes
// the tests consistent and means we can write tests with either separator
const escapedPathSeps = escapeRegExp(`${path.win32.sep}${path.posix.sep}`)

This comment has been minimized.

Copy link
@tannerlinsley

tannerlinsley Apr 16, 2019

Collaborator

Genius

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

This is where the static info is passed into the client bundle: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/bootstrapApp.js#L23-L27

The export process requires the bundle (which is just a commonjs webpack bundle) and calls this function with the siteData, which is then pushed throughout the app via react context.

It's a shame we have to do it this way, but React context instances don't survive across bundles.

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

The actual context instance lives here: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/browser/context/staticInfoContext.js

If the staticInfo context isn't there, then it probably means it's not being passed to the bundle function in export here: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/static/exportRoute.js#L111

Or, it's not being properly embedded into window.__routeInfo here: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/static/components/BodyWithMeta.js#L8

Any thoughts?

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@tannerlinsley For funsies I've changed sharedHashesByProp online line 38 of Routes.js:

        // Hydrate sharedDataByHash with the embedded routeInfo
-       Object.keys(sharedHashesByProp).forEach(propKey => {
+       Object.keys(sharedHashesByProp || {}).forEach(propKey => {
          sharedDataByHash[sharedHashesByProp[propKey]] = sharedData[propKey]
        })

Just so it would pass this part (I did not put any sharedData in the config, so this is fine. It should have been an empty hash if the resolution went correctly). However, now it's saying my static pages are suspending (coming from source-filesystem).

I'm a bit at a loss.

Currently looking through the files you just linked.

SleeplessByte added some commits Apr 16, 2019

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Let me pull in your PR and try it out.

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

In order to reproduce:

  • pull in the PR
  • go into react-static/packages/react-static
  • yarn build
  • go into react-static/packages/react-static-plugin-source-filesystem
  • yarn build

Create a new project with the basic template

  • edit static.config.js
    • comment out the dynamic blog post data (so you only have the static pages left)
    • maxThreads: 1 (so all routes are rendered after one another and you can log stuff in-between)
  • edit package.json dependencies
    • use a relative path to react-static
    • use a relative path to the react-static-plugin-source-filesystem
  • yarn install
  • yarn build

To update the local package:

  • rm yarn.lock
  • make sure you yarn build again in the packages
  • yarn install (this copies the local -relative- package to the local node_modules)
@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Alright. More information!

I've gone ahead and tried to figure out where exactly it starts losing information. So I logged after the line you linked from exportRoute:

{ template: '../src/pages/404.js',
  sharedHashesByProp: {},
  data: {},
  path: '404',
  sharedData: {},
  siteData: {} }

{ template: '../src/pages/about.js',
  sharedHashesByProp: {},
  data: {},
  path: 'about',
  sharedData: {},
  siteData: {} }

{ template: '../src/pages/blog.js',
  sharedHashesByProp: {},
  data: {},
  path: 'blog',
  sharedData: {},
  siteData: {} }

{ template: '../src/pages/index.js',
  sharedHashesByProp: {},
  data: {},
  path: '/',
  sharedData: {},
  siteData: {} }

At this point it's all good! I'm now moving on to figure out if the context is correctly set and retained.

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Awesome! Yeah, once you know the data pipeline, it's relatively easy to track.

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Another possibility is that the static info context is not being externalized during webpack bundling. This would effectively result in 2 copies of the static info context instance: a bundled one, and one that is instantiated during export. This would also conveniently be a slient error...

You could try searching the generated static bundle static-app.js for the context created in staticInfoContext.js. If it is in fact bundled in there, then it's an externalizing bug.

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Again, if that's the bug, then it's a cross-env one like you're thinking.

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

You could try searching the generated static bundle static-app.js for the context created in staticInfoContext.js. If it is in fact bundled in there, then it's an externalizing bug.

What exactly am I looking for? I see static-app.js in my artifacts. It contains bundle 47 which is staticInfoContext:

Object.defineProperty(exports, "staticInfoContext", {
  enumerable: true,
  get: function get() {
    return _staticInfoContext["default"];
  }
});
exports.useStaticInfo = void 0;

var _react = __webpack_require__(0);

var _staticInfoContext = _interopRequireDefault(__webpack_require__(47));

var useStaticInfo = function useStaticInfo() {
  return (0, _react.useContext)(_staticInfoContext["default"]);
};

exports.useStaticInfo = useStaticInfo;

/***/ }),
/* 47 */
/***/ (function(module, exports, __webpack_require__) {

"use strict";
ar _interopRequireDefault = __webpack_require__(6);

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports["default"] = void 0;

var _react = _interopRequireDefault(__webpack_require__(0)); // eslint-disable-next-line


var context = _react["default"].createContext({});

if (typeof document !== 'undefined') {
  context = _react["default"].createContext(window.__routeInfo);
}

var _default = context;
exports["default"] = _default;

...Which is the code to create the context, aka the hook.

This code is minified inside vendors~main.hash.js:

function(e, t, n) {
    "use strict";
    var r = n(2);
    Object.defineProperty(t, "__esModule", {
        value: !0
    }), Object.defineProperty(t, "staticInfoContext", {
        enumerable: !0,
        get: function() {
            return i.default
        }
    }), t.useStaticInfo = void 0;
    var o = n(0),
        i = r(n(103));
    t.useStaticInfo = function() {
        return (0, o.useContext)(i.default)
    }
},
function(e, t, n) {
    "use strict";

    function r(e) {
        return (r = "function" == typeof Symbol && "symbol" == typeof Symbol.iterator ? function(e) {
            return typeof e
        } : function(e) {
            return e && "function" == typeof Symbol && e.constructor === Symbol && e !== Symbol.prototype ? "symbol" : typeof e
        })(e)
    }

    function o(e) {
        return (o = "function" == typeof Symbol && "symbol" === r(Symbol.iterator) ? function(e) {
            return r(e)
        } : function(e) {
            return e && "function" == typeof Symbol && e.constructor === Symbol && e !== Symbol.prototype ? "symbol" : r(e)
        })(e)
    }
    n.r(t);
    var i = n(0),
        a = n.n(i),
        u = (n(111), n(12), n(9)),
        l = n.n(u),
        c = n(47),
        s = n.n(c);


...

function(e, t, n) {
      "use strict";
      var r = n(2);
      Object.defineProperty(t, "__esModule", {
          value: !0
      }), t.default = void 0;
      var o = r(n(0)),
          i = o.default.createContext({});
      "undefined" != typeof document && (i = o.default.createContext(window.__routeInfo));
      var a = i;
      t.default = a
  }
@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Okay. It's okay that it's bundled inside the vendors.main.js file. That's for the client.

The fact that it's inside the static bundle is very bad though and confirms my hunch. Webpack is bundling the staticInfoContext.js file in the node stage (the one that produces static-app.js).

So now the question is, why would it be externalizing that file in my environment, and not yours?

@tannerlinsley

This comment has been minimized.

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Ah; gimme a minute or three. I'll be back with either a fish on a hook or a single tear.

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

🐟 🦈

SleeplessByte added some commits Apr 16, 2019

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@tannerlinsley let's wait for CI; Amended CHANGELOG.md and the description of this PR.

(🎉 🎉 🎉)

There might be more cross-env issues in other plugins, but at least the starter works again. Feel free to ping/tag me whenever you hit one!

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Curiously, are you formatting with the yarn format prettier command?

@SleeplessByte SleeplessByte changed the title react-static-plugin-source-filesystem: Make resolution cross-env Make resolution cross-env in filesystem plugin and base package Apr 16, 2019

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

I think it automatically calls that in my IDE

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Looks like the only thing that needs to be updated is the xml snapshots, which are actually updated in master. Going to merge and go from there!

@tannerlinsley tannerlinsley merged commit cb09081 into react-static:master Apr 16, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
deploy/netlify Deploy preview ready!
Details
@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

For posterity:

This is likely one of the best PR's and Github interactions I have ever had.

@SleeplessByte SleeplessByte deleted the XPBytes:hotfix/cross-platform-plugin-source-filesystem branch Apr 16, 2019

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Thanks for spending the time explaining how all the moving parts work together!

Looks like the only thing that needs to be updated is the xml snapshots, which are actually updated in master. Going to merge and go from there!

Yeah I saw those tests fail -- is that because of a change I made? I didn't touch the XML package so I found it weird. On the other hand; it's of course building a sitemap and I changed paths :P

EDIT: nvm, I see it's failing in that other pr and on travis

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

My pleasure!

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

I updated the XML code in another PR, but not the snapshots.

@tannerlinsley

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Code is now available in the 7.0.8 release!

🎉🎉🎉

@digitalkaoz

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

@SleeplessByte did you test building the site to an absolute path (e.g. /tmp/out)? Its needed for building in Lambda...https://github.com/nozzle/react-static/blob/master/docs/guides/aws-lambda.md

@LukeStanislaus

This comment has been minimized.

Copy link

commented Apr 16, 2019

Hi, I'm still getting the #1122 error in the 7.0.9 release. I'm running through cmd.exe on windows:
image
This is from a completely fresh project. I was able to get it working using the WSL, however. I think that the error is related to cross-env issues.

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Sidenote, in dev I get the following error with the hotloader (cc @tannerlinsley)

Uncaught SyntaxError: The URL 'http:/[http//localhost]:8080' is invalid

Need to look at this weird mangled hot loading webpack-dev-server url'

@digitalkaoz no I did not. Let me know if that currently breaks or not. I'm happy to figure that one out too if it doesnt work!

@LukeStanislaus you're on 7.0.5

Edit: Apparently this was a known issue and has nothing to do with this PR 🗡

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

image

Ha. It's correct once and wrong once 😓 😅

@SleeplessByte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

When I look at the documentation for hot module replacement, it seems the current webpack.dev config file is outdated. I'll open another PR so you can check it out.

Edit: Fixed in #1135

@digitalkaoz

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.