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.

Sign up for GitHub

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

Jump to bottom

Sync up RapiD and iD esbuild config #8776

Merged
merged 6 commits into from
Jan 17, 2022
Merged

Sync up RapiD and iD esbuild config #8776

merged 6 commits into from
Jan 17, 2022

Conversation

bhousel
Copy link
Member

@bhousel bhousel commented Oct 28, 2021

This PR is a best attempt to combine
facebook/Rapid#246 , facebook/Rapid@f415cce

... into a branch that should mostly merge into @mbrzakovic's branch on #8774

Some things that are different are:

  • iD is still not switched over to type: module so the config scripts need a .mjs extention for node to treat them as ES6
  • I didn't include the commands to enter watch mode. We don't have those yet on our end, but it might be useful
  • Babel config is confusing and I don't completely understand it. All the tests pass in Phantom, so I assumed we were covered, but from what you said on Devbuild on esbuild #8774 it sounds like IE11 does not work (I haven't tested it!). This is probably because I only added core-js-bundle to the test index.html 🤔
  • oh I also git removed the generated en.min.json file, because it is annoying that every time I merge anything that generated file has a conflict.

@mbrzakovic
Copy link
Collaborator

This PR is a best attempt to combine facebookincubator#246 , facebookincubator@f415cce

... into a branch that should mostly merge into @mbrzakovic's branch on #8774

Some things that are different are:

  • iD is still not switched over to type: module so the config scripts need a .mjs extention for node to treat them as ES6

mbrzakovic: I am fine with this for now.

  • I didn't include the commands to enter watch mode. We don't have those yet on our end, but it might be useful

mbrzakovic: I think watch mode really speeds up development. I propose we merge this PR as-is, then I add it in devbuild_on_esbuild and after merging you can fetch in upstream. Sounds good?

  • Babel config is confusing and I don't completely understand it. All the tests pass in Phantom, so I assumed we were covered, but from what you said on Devbuild on esbuild #8774 it sounds like IE11 does not work (I haven't tested it!). This is probably because I only added core-js-bundle to the test index.html 🤔

mbrzakovic: Yes, please look into this. If we can't support IE users I'd rather we don't switch to esbuild for legacy build just yet.

  • oh I also git removed the generated en.min.json file, because it is annoying that every time I merge anything that generated file has a conflict.

mbrzakovic: Sounds good.

mbrzakovic
mbrzakovic requested changes Oct 29, 2021
View reviewed changes
babel.config.json Outdated Show resolved Hide resolved
dist/index.html
@@ -32,8 +32,7 @@
newScript.onerror = checkScript;

var isIE11 = !!(navigator.userAgent.match(/Trident/) && !navigator.userAgent.match(/MSIE/));
// currently all minified files are legacy ES5 because of uglifyJS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please help me understand this part by explaining why did we have this constraint (uglifyJS, es5)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was just that UglifyJS never got support for full ES6 syntax, so people use other tools to minify JS nowadays. mishoo/UglifyJS#3443

package.json Outdated Show resolved Hide resolved
@bhousel
Copy link
Member Author

bhousel commented Oct 29, 2021

  • iD is still not switched over to type: module so the config scripts need a .mjs extention for node to treat them as ES6

mbrzakovic: I am fine with this for now.

Cool, yes it is probably a few hours work to add type: module and worth switching at some point, but not essential.
Mainly all the .js scripts need to replace require with import.

  • I didn't include the commands to enter watch mode. We don't have those yet on our end, but it might be useful

mbrzakovic: I think watch mode really speeds up development. I propose we merge this PR as-is, then I add it in devbuild_on_esbuild and after merging you can fetch in upstream. Sounds good?

Yes 👍

  • Babel config is confusing and I don't completely understand it. All the tests pass in Phantom, so I assumed we were covered, but from what you said on Devbuild on esbuild #8774 it sounds like IE11 does not work (I haven't tested it!). This is probably because I only added core-js-bundle to the test index.html 🤔

mbrzakovic: Yes, please look into this. If we can't support IE users I'd rather we don't switch to esbuild for legacy build just yet.

I'll figure it out - I'm sure we can continue to support IE users. My main priority before was keeping the PhantomJS tests running.

@bhousel
Remove babel/preset-react
7c236b3
@bhousel
Just have npm run start start a modern build, remove quickstart
9e0b45d
@bhousel
Better setup for modern/legacy build
4ce17de 
- much simpler babel config
- separate entry points for modern and legacy (ie11/phantom)
- explicitly import 'core-js' and 'regenerator-runtime' instead of using deprecated 'babel/polyfill' plugin
- still need 'core-js-bundle' for the test/index.html, so Mocha/PhantomJS combo will work.
@mbrzakovic mbrzakovic mentioned this pull request Nov 4, 2021
Merged
@mbrzakovic mbrzakovic added the waitfor-info Waiting for more info label Nov 4, 2021
@mbrzakovic
Copy link
Collaborator

Wait for: #8774 (comment)

@tyrasd tyrasd added this to the 2.21.0 milestone Dec 7, 2021
@mbrzakovic mbrzakovic removed the waitfor-info Waiting for more info label Jan 17, 2022
@mbrzakovic
Copy link
Collaborator

Reverted only last commit which improves the modern/legacy build setup.

Reasoning: With Drop IE11 support planned, improvements in supporting legacy build are no longer needed.

@mbrzakovic mbrzakovic merged commit 5040be4 into devbuild_on_esbuild Jan 17, 2022
@mbrzakovic mbrzakovic deleted the more_esbuild branch January 17, 2022 13:18
tyrasd added a commit that referenced this pull request Feb 3, 2022
@tyrasd
drop en.min.json which was (accidentally?) added again after #8776
d931102
@tyrasd tyrasd mentioned this pull request Apr 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrzakovic mbrzakovic mbrzakovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.21.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bhousel @mbrzakovic @tyrasd