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

v3 #839

Merged
merged 26 commits into from
Apr 16, 2019
Merged

v3 #839

merged 26 commits into from
Apr 16, 2019

Conversation

jaredpalmer
Copy link
Owner

@jaredpalmer jaredpalmer commented Nov 14, 2018

lucasterra and others added 7 commits November 14, 2018 09:41
* Upgrade Babel to v7, Jest to v23.6

Fix the following examples

* with-custom-babel-config
* with-inferno
* with-preact
* with-react-native-web
* with-rax

* Mimic other CRA defaults
"mini-css-extract-plugin": "^0.4.0",
"@babel/core": "7.1.2",
"assets-webpack-plugin": "3.9.7",
"babel-core": "^7.0.0-bridge.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredpalmer is babel-core required if have @babel/core already?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcneander
Copy link
Contributor

#845

@olehreznichenko
Copy link
Contributor

I see this warning in console ->
Warning: startReportingRuntimeErrors doesn’t accept launchEditorEndpoint argument anymore. Use listenToOpenInEditor instead with your own implementation to open errors in editor
Comes from react-error-overlay

@Dakkers
Copy link

Dakkers commented Dec 1, 2018

@jaredpalmer what do you think the timeline is for v3?

@jaredpalmer
Copy link
Owner Author

Waiting for people to give feedback.

@dan-lee
Copy link

dan-lee commented Dec 2, 2018

I just dropped 3.0.0-alpha.0 in a medium sized page and it works like a charm.

But I also see this warning:

Warning: startReportingRuntimeErrors doesn’t accept launchEditorEndpoint argument anymore. Use listenToOpenInEditor instead with your own implementation to open errors in editor

@olehreznichenko
Copy link
Contributor

olehreznichenko commented Dec 2, 2018

Was playing with @babel/preset-env and found two problems
The idea is to override default preset-env in razzle in https://github.com/jaredpalmer/razzle/blob/next/packages/babel-preset-razzle/index.js

  1. Create new app with next razzle, then create .babelrc ->
{
  "presets": [
    "razzle/babel",
    [
      "@babel/preset-env",
      {
        "useBuiltIns": "usage",
        "loose": true,
        "targets": {
          "node": "current",
          "browsers": ["chrome 70"]
        }
      }
    ]
  ]
}

Run build script and will see this
Failed to compile.

./src/Home.js
Module not found: Can't resolve '@babel/runtime/helpers/inheritsLoose'
Fixed after adding @babel/runtime as the dependency

  1. I think it`s more like babel issue
"targets": {
    "node": "current",
   "browsers": ["chrome 70"]
}

is working as expected, but if change chrome version to 41 for example. server.js will be transformed also to old code but in my machine installed node v11

@jaredpalmer
Copy link
Owner Author

Do you have a suggested fix?

@olehreznichenko
Copy link
Contributor

Fix for the second option? - not yet

@jaredpalmer
Copy link
Owner Author

@dan-lee can you submit a PR?

@lfantone
Copy link

lfantone commented Dec 4, 2018

We upgrade one of our internal razzle app to thee alpha release, so far we don't have any mayor issue with it.
What I saw is the same warming message that @dan-lee already reported.

@hrasoa
Copy link
Contributor

hrasoa commented Dec 4, 2018

@olegreznichenko @jaredpalmer there's already a PR for @babel/runtime: #845

@Dakkers
Copy link

Dakkers commented Dec 4, 2018

I don't mind helping test out if possible. how do I go about setting up a new app with v3? can I use npx for it?

@hrasoa
Copy link
Contributor

hrasoa commented Dec 11, 2018

@jaredpalmer could you re-tag and include #845 ?

@oyvinmar
Copy link
Contributor

I just upgraded one of our frontends without any issues. Only the previous reported warning message.

Would be nice with a new alpha release with the polyfill removal (we are working on a solution for conditionally including polyfills).

@andrefox333
Copy link

Waiting patiently for this version to be released!

@jaredpalmer
Copy link
Owner Author

Will cut a release later today

@rg-najera
Copy link

rg-najera commented Jan 7, 2019

@jaredpalmer I have a little something for ya. Seems like "Transpile node_modules with babel (like CRA does)" didn't get crossed off the list. I have a working version here ready if interested. Needs to be rebased with the new alpha. I wont have time this week to tie it off but if someone whats to pick it up, I'' post the PR. rg-najera#1

@jaredpalmer
Copy link
Owner Author

Yeah, I don't use babel so I never got around to it. So if someone wants to tackle that, it would be rad.

@rg-najera
Copy link

Copy that @jaredpalmer I'll post the PR and can tie up loose ends for merge. Is there another exit criteria for moving out of alpha?

@gribnoysup
Copy link

babel@7 supports different file names for the config: .babelrc, babel.config.js, .babelrc.js. Seems like razzle@next is still looking only for .babelrc in project root when deciding when to override the default babel configuration. Is this intended? 🤔

@rg-najera
Copy link

@gribnoysup when working on this last, I did need to make an adjustment so that babelrc.js is picked up. As @jaredpalmer noted, he doesn't use babel so that was probably unintentional. Will make a note and add support for all.

@gribnoysup
Copy link

@el-rotny ahh, totally makes sense, thanks for clarifying! 😁

@hrasoa
Copy link
Contributor

hrasoa commented Jan 13, 2019

I'm working on a medium sized project and everything is working fine so far on v3.0.0-alpha.2

@bartlangelaan bartlangelaan mentioned this pull request Jan 21, 2019
@mschipperheyn
Copy link
Contributor

mschipperheyn commented Jan 24, 2019

Migrated a react-universally based sequelize application with large admin to razzle alpha 2 in about 2.5 days.

Nice:

  • development reloads were fast
  • deployment was much faster

Issues:

  • Personally not a fan of the "put everything (jest etc) in package.json mostly because of commentability, but I understand the wish to keep aligned with CRA.
  • No vendor bundle but still fast development reloads
  • No integrated bundle analyzer, sourly missed.
  • Lack of CSP support. Implemented the react-universally version which is pretty good. Security considerations should imho be a bigger concern.
  • Created a custom workaround for react-async-component which is failing now bc of an unmerged critical update. Surprised there is no example for this library which seems far superior to react-loadable and loadable-components. I will prob contribute one when @ctrlplusb releases a new version
  • Needed to create an undocumented "postinstall": "yarn build" yarn command to get razzle to build correctly during deployment.
  • deployment to dokku production bc the port wasn't exposed, lead to failed production deploy: Compiled build inlines PORT environment variable #356 (comment) helped. dokku defaults to 5000, razzle to 3000. So that was prob it. Maybe it just worked and I didn't realize bc of other issues.
  • Razzle src/index script should report the PORT it's running on to facilitate identifying errors console.log(🚀 started on port ${env.get('PORT')});
  • Saw a bunch of errors that may have been caused by the serviceworker of the "old version". I wonder if the serviceworker is smart enough to detect a large scale update? Could we be having an issue on the side of the user without realizing?
  • Have a problem with bundle js and css being served as text/html bc of a 404. So, prob also good to console.log the process.env.RAZZLE_PUBLIC_DIR during startup. Yeah, it gets set wrong and you have to add it to dokku/heroku ENV: RAZZLE_PUBLIC_DIR=build/public. Heroku example helped a lot, but this (trying to figure out how to set this var correctly) is basically a sucky way of doing things. Especially, since RAZZLE_PUBLIC_DIR is a build time variable. This can be better.
  • Had to do some hard cache refreshes to get the accurate js and css. It seems that 404 responses are being cached. That could be my server setup or it could be razzle. I also use CloudFlare and clean the cache after each deploy. Not sure.
    Annnnnddddd, it's up.

@jaredpalmer
Copy link
Owner Author

@mschipperheyn Agree about security. We could add helmet or lusca to all the templates? I was trying to keep all the examples "matching the docs" of the given example's concept. So perhaps a better solution would be to document additional security considerations and let people do it themselves?

@mschipperheyn
Copy link
Contributor

mschipperheyn commented Jan 24, 2019

@jaredpalmer I implemented react-helmet-async for decentralized header update and helmet for csp. I use a slightly modified for razzle react-universally config (https://github.com/ctrlplusb/react-universally/blob/next/server/middleware/security.js). What react-universally did very nicely imho is have centralized config file (https://github.com/ctrlplusb/react-universally/blob/next/config/values.js) with sensible default switches for things like serviceworkers, csp etc. I also partly copied that feature over to my razzle config. Also helps to keep those process.env.VARIABLE from being all over the place.

Not familiar with lusca but it looks good.

When it comes to security, on the one hand I think you should help people be protected out of the box. On the other hand, csp will lead to a number of support questions because things may not load as expected if you make a mistake. Perhaps it should be a strongly recommnended plugin with clear docs on the kinds of issues you might run into.

@mschipperheyn
Copy link
Contributor

Oh, one thing that might be worth mentioning is what I did to deal with async startup services. In my case, I want the sequelize and redis service to be up before we start accepting requests. I solved this as such, not sure if it could be better:

src/index.js

const serverImport = require('./server');
const port = config('port');
let server = serverImport.default;
let app = serverImport.app;
let preStart = serverImport.preStart;

let currentApp = app;

preStart(() => {
	server.listen(port || 3000, error => {
		if (error) {
			console.log(error);
		}

		console.log(`🚀 started on port ${port}`);
	});
});

if (module.hot) {
	console.log('✅  Server-side HMR Enabled!');

	module.hot.accept('./server', () => {
		console.log('🔁  HMR Reloading `./server`...');

		try {
			let app = require('./server').app;
			server.removeListener('request', currentApp);
			server.on('request', app);
			currentApp = app;
		} catch (error) {
			console.error(error);
		}
	});
}

src/server/js

[...]
const preStart = async cb => {
	try {
		await testConnection();

		await sequelize.sync({ hooks: true });

		cb();
	} catch (err) {
		console.error(err);
	}
};

const closeDatabase = async () => {
	await stopRedis();
	await close();
};

const shutdown = async () => {
	console.log('Received kill signal, shutting down gracefully.');
	await server.close(async () => {
		console.log('Closed out remaining connections');
		await closeDatabase();
		process.exit(0);
	});

	await setTimeout(async () => {
		console.error(
			'Could not close connections in time, forcefully shutting down'
		);
		await closeDatabase();
		process.exit(1);
	}, 10000);
};

process.on('SIGTERM', shutdown);
process.on('SIGINT', shutdown);

export { server as default, preStart, app };

@mschipperheyn
Copy link
Contributor

mschipperheyn commented Jan 24, 2019

Another issue I noticed is that the TerserPlugin is not removing some obvious wins.
I'm no expert on this, but using

new TerserPlugin({
    terserOptions: {
        global_defs: {
            'process.env.NODE_ENV': 'production',
        },

should drop all the if(process.env.NODE_ENV === 'development') sections. Should be a nice win.

Tried this out, and made no difference. I guess TerserPlugin already does this

@mschipperheyn
Copy link
Contributor

It would also be nice to have some kind of dynamic integration with polyfill.io. https://github.com/facebook/create-react-app/tree/master/packages/react-app-polyfill seems very static. In an SSR scenario, it should be possible to dynamically pass the right polyfills config down the line and reference polyfill.io

@jaredpalmer
Copy link
Owner Author

@mschipperheyn that’s a brilliant idea. Seems like it could be done as a plugin while we incubate it?

@jaredpalmer
Copy link
Owner Author

@mschipperheyn can you whip some razzle examples for react-helmet-async, sequelize, and redis?

@mschipperheyn
Copy link
Contributor

mschipperheyn commented Jan 25, 2019

@jaredpalmer yes, I can do that. #899

@mschipperheyn
Copy link
Contributor

Some weirdness I'm running into since I migrated to Razzle is that I get this create-react-app style non visible error overlay, which doesn't display anything, but in the element source shows an inlined javascript error, completely escape in unreadable form.

screenshot 2019-01-25 at 13 45 51

bartlangelaan and others added 2 commits February 8, 2019 15:48
* Remove eslint handling from razzle-plugin-typescript

* Remove parts from docs mentioning eslint

* Move eslint specific code over to razzle-plugin-eslint

* Add a temporary eslint-plugin-react setting

* Update to the latest eslint packages as defined in react-scripts
…#903)

* add globalSetup, globalTeardown and moduleDirectories to jest allowed

* rerun tests
@andrefox333
Copy link

@jaredpalmer can you cut a new alpha version? I'm interested in the webpack-dev-server 3 piece. :) Thanks man.

@jaredpalmer
Copy link
Owner Author

Cutting a release right meow

@jaredpalmer jaredpalmer merged commit b089543 into master Apr 16, 2019
@andrefox333
Copy link

Thanks @jaredpalmer, it works. I ran into some webpack import issue using react-loadable. Same issue that people are running into here:

webpack/webpack#8656

and solved the issue by following this comment:
webpack/webpack#8656 (comment)

@jlaustill
Copy link

Ok, I converted my cra app to razzle 3, but tree shaking is not happening. I can't find anything in the docs about it. Is there something I need to configure?

Other than that, I am loving razzle 3!

@nimaa77
Copy link
Collaborator

nimaa77 commented Sep 20, 2019

@jlaustill same here, I fixed it by just adding sideEffects: ["*.css"] to my package.json.
but it must happen automatically, IDK why it behaves like this!

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

Successfully merging this pull request may close these issues.